Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
eba4f92
record security exceptions in resolved index expressions
richard-dennehy Sep 29, 2025
e5e3e12
[CI] Auto commit changes from spotless
Sep 29, 2025
d2f5563
override equality in LocalExpressions
richard-dennehy Sep 29, 2025
c236591
merge main
richard-dennehy Oct 7, 2025
e7e8c16
[CI] Auto commit changes from spotless
Oct 7, 2025
6c302bd
record exception for unresolved index when ignore_unavailable is true
richard-dennehy Oct 9, 2025
359fca7
clean up
richard-dennehy Oct 9, 2025
1fdb0a8
spotless
richard-dennehy Oct 9, 2025
4a3ffd4
Merge remote-tracking branch 'upstream/main' into resolved-index-expr…
richard-dennehy Oct 10, 2025
98a50b9
enable CPS mode in test
richard-dennehy Oct 10, 2025
9d89502
Merge remote-tracking branch 'upstream/main' into resolved-index-expr…
richard-dennehy Oct 13, 2025
f143685
include index name in exception message
richard-dennehy Oct 13, 2025
6109ecb
[CI] Auto commit changes from spotless
Oct 13, 2025
b81d263
merge main
richard-dennehy Oct 24, 2025
69e46f3
address review comments
richard-dennehy Oct 24, 2025
a0e594f
[CI] Auto commit changes from spotless
Oct 24, 2025
d99f487
assert exception is not set twice
richard-dennehy Oct 24, 2025
dfb3081
use recorded exception in CrossProjectIndexResolutionValidator
richard-dennehy Oct 27, 2025
38115ad
[CI] Auto commit changes from spotless
Oct 27, 2025
88f63a3
actually only set exception if unset
richard-dennehy Oct 28, 2025
9aef86f
Merge branch 'main' into resolved-index-expression-record-exceptions
richard-dennehy Oct 28, 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 @@ -16,6 +16,7 @@
import org.elasticsearch.core.Nullable;

import java.io.IOException;
import java.util.Objects;
import java.util.Set;

/**
Expand Down Expand Up @@ -76,14 +77,72 @@ public enum LocalIndexResolutionResult {
/**
* Represents local (non-remote) resolution results, including expanded indices, and a {@link LocalIndexResolutionResult}.
*/
public record LocalExpressions(
Set<String> expressions,
LocalIndexResolutionResult localIndexResolutionResult,
@Nullable ElasticsearchException exception
) implements Writeable {
public LocalExpressions {
public static final class LocalExpressions implements Writeable {
private final Set<String> expressions;
private final LocalIndexResolutionResult localIndexResolutionResult;
@Nullable
private ElasticsearchException exception;

public LocalExpressions(
Set<String> expressions,
LocalIndexResolutionResult localIndexResolutionResult,
@Nullable ElasticsearchException exception
) {
assert localIndexResolutionResult != LocalIndexResolutionResult.SUCCESS || exception == null
: "If the local resolution result is SUCCESS, exception must be null";
this.expressions = expressions;
this.localIndexResolutionResult = localIndexResolutionResult;
this.exception = exception;
}

public Set<String> expressions() {
return expressions;
}

public LocalIndexResolutionResult localIndexResolutionResult() {
return localIndexResolutionResult;
}

@Nullable
public ElasticsearchException exception() {
return exception;
}

public void setException(ElasticsearchException exception) {
assert localIndexResolutionResult != LocalIndexResolutionResult.SUCCESS
: "If the local resolution result is SUCCESS, exception must be null";
Objects.requireNonNull(exception);

this.exception = exception;
Copy link
Member

Choose a reason for hiding this comment

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

We should also assert this.exception is null before set it

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 causing the CPS Rest IT to fail - I need to figure out why that's happening

Copy link
Member

Choose a reason for hiding this comment

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

Ah OK. I think this is the same double security filtering issue which required the method setResolvedIndexExpressionsIfUnset.

We can do something similar, i.e. set the exception when this.exception is null. If this.exception is already set, assert the two exceptions have the same type and error message (stacktraces are not necessarily the same).

}

@Override
public boolean equals(Object obj) {
if (obj == this) return true;
if (obj == null || obj.getClass() != this.getClass()) return false;
var that = (LocalExpressions) obj;
return Objects.equals(this.expressions, that.expressions)
&& Objects.equals(this.localIndexResolutionResult, that.localIndexResolutionResult)
&& Objects.equals(this.exception, that.exception);
}

@Override
public int hashCode() {
return Objects.hash(expressions, localIndexResolutionResult, exception);
}

@Override
public String toString() {
return "LocalExpressions["
+ "expressions="
+ expressions
+ ", "
+ "localIndexResolutionResult="
+ localIndexResolutionResult
+ ", "
+ "exception="
+ exception
+ ']';
}

// Singleton for the case where all expressions in a ResolvedIndexExpression instance are remote
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.DelegatingActionListener;
import org.elasticsearch.action.DocWriteRequest;
import org.elasticsearch.action.IndicesRequest;
import org.elasticsearch.action.admin.indices.alias.Alias;
import org.elasticsearch.action.admin.indices.alias.TransportIndicesAliasesAction;
import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
Expand Down Expand Up @@ -108,6 +109,7 @@
import java.util.function.Consumer;
import java.util.function.Supplier;

import static org.elasticsearch.action.ResolvedIndexExpression.LocalIndexResolutionResult.CONCRETE_RESOURCE_UNAUTHORIZED;
import static org.elasticsearch.action.support.ContextPreservingActionListener.wrapPreservingContext;
import static org.elasticsearch.xpack.core.security.SecurityField.setting;
import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.ACTION_SCOPE_AUTHORIZATION_KEYS;
Expand Down Expand Up @@ -689,6 +691,17 @@ private void handleIndexActionAuthorizationResult(
)
);
} else {
if (request instanceof IndicesRequest.Replaceable replaceable) {
var indexExpressions = replaceable.getResolvedIndexExpressions();
if (indexExpressions != null) {
indexExpressions.expressions().forEach(resolved -> {
if (resolved.localExpressions().localIndexResolutionResult() == CONCRETE_RESOURCE_UNAUTHORIZED) {
resolved.localExpressions().setException(actionDenied(authentication, authzInfo, action, request));
}
});
}
}

runRequestInterceptors(requestInfo, authzInfo, authorizationEngine, listener);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -406,9 +406,9 @@ ResolvedIndices resolveIndicesAndAliases(
// only store resolved expressions if configured, to avoid unnecessary memory usage
// once we've migrated from `indices()` to using resolved expressions holistically,
// we will always store them
if (crossProjectModeDecider.crossProjectEnabled()) {
setResolvedIndexExpressionsIfUnset(replaceable, resolved);
}
// if (crossProjectModeDecider.crossProjectEnabled()) {
setResolvedIndexExpressionsIfUnset(replaceable, resolved);
// }
resolvedIndicesBuilder.addLocal(resolved.getLocalIndicesList());
resolvedIndicesBuilder.addRemote(split.getRemote());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*/
package org.elasticsearch.xpack.security.authz;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.TransportVersion;
import org.elasticsearch.action.ActionListener;
Expand All @@ -14,6 +15,7 @@
import org.elasticsearch.action.LatchedActionListener;
import org.elasticsearch.action.MockIndicesRequest;
import org.elasticsearch.action.OriginalIndices;
import org.elasticsearch.action.ResolvedIndexExpression;
import org.elasticsearch.action.admin.cluster.health.ClusterHealthRequest;
import org.elasticsearch.action.admin.cluster.health.TransportClusterHealthAction;
import org.elasticsearch.action.admin.indices.alias.Alias;
Expand Down Expand Up @@ -216,6 +218,8 @@
import java.util.function.Supplier;

import static java.util.Arrays.asList;
import static org.elasticsearch.action.ResolvedIndexExpression.LocalIndexResolutionResult.CONCRETE_RESOURCE_UNAUTHORIZED;
import static org.elasticsearch.action.ResolvedIndexExpression.LocalIndexResolutionResult.SUCCESS;
import static org.elasticsearch.test.ActionListenerUtils.anyActionListener;
import static org.elasticsearch.test.ActionListenerUtils.anyCollection;
import static org.elasticsearch.test.SecurityTestsUtils.assertAuthenticationException;
Expand All @@ -236,7 +240,9 @@
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
import static org.hamcrest.Matchers.arrayWithSize;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.not;
Expand Down Expand Up @@ -3690,6 +3696,43 @@ public void testRoleRestrictionAccessDenial() {
assertThat(securityException.getRootCause(), throwableWithMessage(containsString("access restricted by workflow")));
}

public void testSetExceptionOnMissingIndexWhenIgnoreUnavailable() {
mockMetadataWithIndex("available-index");
Copy link
Member

Choose a reason for hiding this comment

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

The deciding factor here is that the user has no access to the index regardless whether it exists or not. So I'd suggest:

  1. rename available-index and not-available-index to accessible-index and not-accessible-index.
  2. Randomly also add not-accessible-index to the project metadata. The behaviour should not change whether this index exists or not
  3. random between indices request between accessible-index,not-accessible-index and just not-accessible-index. The behaviour should be the same.

var authentication = createAuthentication(new User("user", "partial-access-role"));
roleMap.put(
"partial-access-role",
new RoleDescriptor(
"partial-access-role",
null,
new IndicesPrivileges[] { IndicesPrivileges.builder().indices("available-index").privileges("read").build() },
null
)
);
final var request = new SearchRequest("available-index", "not-available-index").indicesOptions(
IndicesOptions.fromOptions(true, false, true, false)
);
AuditUtil.getOrGenerateRequestId(threadContext);
authorize(authentication, TransportSearchAction.TYPE.name(), request);

final var authorizationInfo = mock(AuthorizationInfo.class);
when(authorizationInfo.asMap()).thenReturn(Map.of("user.info", new String[] { "partial-access-role" }));

final var expressions = request.getResolvedIndexExpressions().expressions();
assertThat(expressions, hasSize(2));
assertThat(expressions.getFirst(), equalTo(resolvedIndexExpression("available-index", Set.of("available-index"), SUCCESS)));

assertThat(expressions.get(1).original(), equalTo("not-available-index"));
assertThat(expressions.get(1).localExpressions().expressions(), empty());
assertThat(expressions.get(1).localExpressions().localIndexResolutionResult(), equalTo(CONCRETE_RESOURCE_UNAUTHORIZED));
assertThat(
expressions.get(1).localExpressions().exception().getMessage(),
equalTo(
"action [indices:data/read/search] is unauthorized for user [user] with effective roles [partial-access-role], "
+ "this action is granted by the index privileges [read,all]"
)
Comment on lines 3916 to 3920
Copy link
Member

Choose a reason for hiding this comment

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

We should assert the index name is included in the error message

);
}

static AuthorizationInfo authzInfoRoles(String[] expectedRoles) {
return argThat(new RBACAuthorizationInfoRoleMatcher(expectedRoles));
}
Expand Down Expand Up @@ -3748,4 +3791,29 @@ public String getWriteableName() {
@Override
public void writeTo(StreamOutput out) throws IOException {}
}

private static ResolvedIndexExpression resolvedIndexExpression(
String original,
Set<String> localExpressions,
ResolvedIndexExpression.LocalIndexResolutionResult localIndexResolutionResult
) {
return new ResolvedIndexExpression(
original,
new ResolvedIndexExpression.LocalExpressions(localExpressions, localIndexResolutionResult, null),
Set.of()
);
}

private static ResolvedIndexExpression resolvedIndexExpression(
String original,
Set<String> localExpressions,
ResolvedIndexExpression.LocalIndexResolutionResult localIndexResolutionResult,
ElasticsearchException exception
) {
return new ResolvedIndexExpression(
original,
new ResolvedIndexExpression.LocalExpressions(localExpressions, localIndexResolutionResult, exception),
Set.of()
);
}
}