Skip to content

Commit e2b3f57

Browse files
richard-dennehyelasticsearchmachine
authored and
chrisparrinello
committed
record security exceptions in resolved index expressions (elastic#135630)
* record security exceptions in resolved index expressions * [CI] Auto commit changes from spotless * override equality in LocalExpressions * [CI] Auto commit changes from spotless * record exception for unresolved index when ignore_unavailable is true * clean up * spotless * enable CPS mode in test * include index name in exception message * [CI] Auto commit changes from spotless * address review comments * [CI] Auto commit changes from spotless * assert exception is not set twice * use recorded exception in CrossProjectIndexResolutionValidator * [CI] Auto commit changes from spotless * actually only set exception if unset --------- Co-authored-by: elasticsearchmachine <[email protected]>
1 parent 5a7f4b0 commit e2b3f57

File tree

5 files changed

+235
-42
lines changed

5 files changed

+235
-42
lines changed

‎server/src/main/java/org/elasticsearch/action/ResolvedIndexExpression.java‎

Lines changed: 79 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,16 @@
99

1010
package org.elasticsearch.action;
1111

12+
import org.apache.logging.log4j.LogManager;
13+
import org.apache.logging.log4j.Logger;
1214
import org.elasticsearch.ElasticsearchException;
1315
import org.elasticsearch.common.io.stream.StreamInput;
1416
import org.elasticsearch.common.io.stream.StreamOutput;
1517
import org.elasticsearch.common.io.stream.Writeable;
1618
import org.elasticsearch.core.Nullable;
1719

1820
import java.io.IOException;
21+
import java.util.Objects;
1922
import java.util.Set;
2023

2124
/**
@@ -49,6 +52,8 @@ public record ResolvedIndexExpression(String original, LocalExpressions localExp
4952
implements
5053
Writeable {
5154

55+
private static final Logger logger = LogManager.getLogger(ResolvedIndexExpression.class);
56+
5257
public ResolvedIndexExpression(StreamInput in) throws IOException {
5358
this(in.readString(), new LocalExpressions(in), in.readCollectionAsImmutableSet(StreamInput::readString));
5459
}
@@ -76,17 +81,83 @@ public enum LocalIndexResolutionResult {
7681

7782
/**
7883
* Represents local (non-remote) resolution results, including expanded indices, and a {@link LocalIndexResolutionResult}.
79-
*
80-
* @param indices represents the resolved concrete indices backing the expression
8184
*/
82-
public record LocalExpressions(
83-
Set<String> indices,
84-
LocalIndexResolutionResult localIndexResolutionResult,
85-
@Nullable ElasticsearchException exception
86-
) implements Writeable {
87-
public LocalExpressions {
85+
public static final class LocalExpressions implements Writeable {
86+
private final Set<String> indices;
87+
private final LocalIndexResolutionResult localIndexResolutionResult;
88+
@Nullable
89+
private ElasticsearchException exception;
90+
91+
/**
92+
* @param indices represents the resolved concrete indices backing the expression
93+
*/
94+
public LocalExpressions(
95+
Set<String> indices,
96+
LocalIndexResolutionResult localIndexResolutionResult,
97+
@Nullable ElasticsearchException exception
98+
) {
8899
assert localIndexResolutionResult != LocalIndexResolutionResult.SUCCESS || exception == null
89100
: "If the local resolution result is SUCCESS, exception must be null";
101+
this.indices = indices;
102+
this.localIndexResolutionResult = localIndexResolutionResult;
103+
this.exception = exception;
104+
}
105+
106+
public Set<String> indices() {
107+
return indices;
108+
}
109+
110+
public LocalIndexResolutionResult localIndexResolutionResult() {
111+
return localIndexResolutionResult;
112+
}
113+
114+
@Nullable
115+
public ElasticsearchException exception() {
116+
return exception;
117+
}
118+
119+
public void setExceptionIfUnset(ElasticsearchException exception) {
120+
assert localIndexResolutionResult != LocalIndexResolutionResult.SUCCESS
121+
: "If the local resolution result is SUCCESS, exception must be null";
122+
Objects.requireNonNull(exception);
123+
124+
if (this.exception == null) {
125+
this.exception = exception;
126+
} else if (Objects.equals(this.exception.getMessage(), exception.getMessage()) == false) {
127+
// see https://github.com/elastic/elasticsearch/issues/135799
128+
var message = "Exception is already set: " + exception.getMessage();
129+
logger.debug(message);
130+
assert false : message;
131+
}
132+
}
133+
134+
@Override
135+
public boolean equals(Object obj) {
136+
if (obj == this) return true;
137+
if (obj == null || obj.getClass() != this.getClass()) return false;
138+
var that = (LocalExpressions) obj;
139+
return Objects.equals(this.indices, that.indices)
140+
&& Objects.equals(this.localIndexResolutionResult, that.localIndexResolutionResult)
141+
&& Objects.equals(this.exception, that.exception);
142+
}
143+
144+
@Override
145+
public int hashCode() {
146+
return Objects.hash(indices, localIndexResolutionResult, exception);
147+
}
148+
149+
@Override
150+
public String toString() {
151+
return "LocalExpressions["
152+
+ "indices="
153+
+ indices
154+
+ ", "
155+
+ "localIndexResolutionResult="
156+
+ localIndexResolutionResult
157+
+ ", "
158+
+ "exception="
159+
+ exception
160+
+ ']';
90161
}
91162

92163
// Singleton for the case where all expressions in a ResolvedIndexExpression instance are remote

‎server/src/main/java/org/elasticsearch/search/crossproject/CrossProjectIndexResolutionValidator.java‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -98,7 +98,7 @@ public static ElasticsearchException validate(
9898
ResolvedIndexExpression.LocalExpressions localExpressions = localResolvedIndices.localExpressions();
9999
ResolvedIndexExpression.LocalIndexResolutionResult result = localExpressions.localIndexResolutionResult();
100100
if (isQualifiedExpression) {
101-
ElasticsearchException e = checkResolutionFailure(localExpressions.indices(), result, originalExpression, indicesOptions);
101+
ElasticsearchException e = checkResolutionFailure(localExpressions, result, originalExpression, indicesOptions);
102102
if (e != null) {
103103
return e;
104104
}
@@ -118,7 +118,7 @@ public static ElasticsearchException validate(
118118
}
119119
} else {
120120
ElasticsearchException localException = checkResolutionFailure(
121-
localExpressions.indices(),
121+
localExpressions,
122122
result,
123123
originalExpression,
124124
indicesOptions
@@ -152,7 +152,7 @@ public static ElasticsearchException validate(
152152
continue;
153153
}
154154
if (isUnauthorized) {
155-
return securityException(originalExpression);
155+
return localException;
156156
}
157157
return new IndexNotFoundException(originalExpression);
158158
}
@@ -191,7 +191,7 @@ private static ElasticsearchException checkSingleRemoteExpression(
191191
}
192192

193193
return checkResolutionFailure(
194-
matchingExpression.indices(),
194+
matchingExpression,
195195
matchingExpression.localIndexResolutionResult(),
196196
remoteExpression,
197197
indicesOptions
@@ -223,7 +223,7 @@ private static ResolvedIndexExpression.LocalExpressions findMatchingExpression(
223223
}
224224

225225
private static ElasticsearchException checkResolutionFailure(
226-
Set<String> localExpressions,
226+
ResolvedIndexExpression.LocalExpressions localExpressions,
227227
ResolvedIndexExpression.LocalIndexResolutionResult result,
228228
String expression,
229229
IndicesOptions indicesOptions
@@ -235,11 +235,14 @@ private static ElasticsearchException checkResolutionFailure(
235235
if (result == CONCRETE_RESOURCE_NOT_VISIBLE) {
236236
return new IndexNotFoundException(expression);
237237
} else if (result == CONCRETE_RESOURCE_UNAUTHORIZED) {
238-
return securityException(expression);
238+
assert localExpressions.exception() != null
239+
: "ResolvedIndexExpression should have exception set when concrete index is unauthorized";
240+
241+
return localExpressions.exception();
239242
}
240243
}
241244

242-
if (indicesOptions.allowNoIndices() == false && result == SUCCESS && localExpressions.isEmpty()) {
245+
if (indicesOptions.allowNoIndices() == false && result == SUCCESS && localExpressions.indices().isEmpty()) {
243246
return new IndexNotFoundException(expression);
244247
}
245248

‎server/src/test/java/org/elasticsearch/search/crossproject/CrossProjectIndexResolutionValidatorTests.java‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323

2424
import static org.hamcrest.Matchers.containsString;
2525
import static org.hamcrest.Matchers.instanceOf;
26+
import static org.hamcrest.Matchers.is;
2627

2728
public class CrossProjectIndexResolutionValidatorTests extends ESTestCase {
2829

@@ -124,14 +125,15 @@ public void testMissingFlatExpressionWithStrictIgnoreUnavailable() {
124125
}
125126

126127
public void testUnauthorizedFlatExpressionWithStrictIgnoreUnavailable() {
128+
final var exception = new ElasticsearchSecurityException("authorization errors while resolving [logs]");
127129
ResolvedIndexExpressions local = new ResolvedIndexExpressions(
128130
List.of(
129131
new ResolvedIndexExpression(
130132
"logs",
131133
new ResolvedIndexExpression.LocalExpressions(
132134
Set.of(),
133135
ResolvedIndexExpression.LocalIndexResolutionResult.CONCRETE_RESOURCE_UNAUTHORIZED,
134-
new ElasticsearchSecurityException("authorization errors while resolving [logs]")
136+
exception
135137
),
136138
Set.of("P1:logs")
137139
)
@@ -157,8 +159,7 @@ public void testUnauthorizedFlatExpressionWithStrictIgnoreUnavailable() {
157159

158160
var e = CrossProjectIndexResolutionValidator.validate(getStrictIgnoreUnavailable(), local, remote);
159161
assertNotNull(e);
160-
assertThat(e, instanceOf(ElasticsearchSecurityException.class));
161-
assertThat(e.getMessage(), containsString("user cannot access [logs]"));
162+
assertThat(e, is(exception));
162163
}
163164

164165
public void testQualifiedExpressionWithStrictIgnoreUnavailableMatchingInOriginProject() {
@@ -270,6 +271,7 @@ public void testUnauthorizedQualifiedExpressionWithStrictIgnoreUnavailable() {
270271
List.of(new ResolvedIndexExpression("P1:logs", ResolvedIndexExpression.LocalExpressions.NONE, Set.of("P1:logs")))
271272
);
272273

274+
final var exception = new ElasticsearchException("action is unauthorized for indices [logs]");
273275
var remote = Map.of(
274276
"P1",
275277
new ResolvedIndexExpressions(
@@ -279,7 +281,7 @@ public void testUnauthorizedQualifiedExpressionWithStrictIgnoreUnavailable() {
279281
new ResolvedIndexExpression.LocalExpressions(
280282
Set.of(),
281283
ResolvedIndexExpression.LocalIndexResolutionResult.CONCRETE_RESOURCE_UNAUTHORIZED,
282-
new ElasticsearchException("logs")
284+
exception
283285
),
284286
Set.of()
285287
)
@@ -289,8 +291,7 @@ public void testUnauthorizedQualifiedExpressionWithStrictIgnoreUnavailable() {
289291

290292
var e = CrossProjectIndexResolutionValidator.validate(getStrictIgnoreUnavailable(), local, remote);
291293
assertNotNull(e);
292-
assertThat(e, instanceOf(ElasticsearchSecurityException.class));
293-
assertThat(e.getMessage(), containsString("user cannot access [P1:logs]"));
294+
assertThat(e, is(exception));
294295
}
295296

296297
public void testFlatExpressionWithStrictAllowNoIndicesMatchingInOriginProject() {

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java‎

Lines changed: 43 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@
1616
import org.elasticsearch.action.ActionListener;
1717
import org.elasticsearch.action.DelegatingActionListener;
1818
import org.elasticsearch.action.DocWriteRequest;
19+
import org.elasticsearch.action.IndicesRequest;
1920
import org.elasticsearch.action.admin.indices.alias.Alias;
2021
import org.elasticsearch.action.admin.indices.alias.TransportIndicesAliasesAction;
2122
import org.elasticsearch.action.admin.indices.create.CreateIndexRequest;
@@ -108,6 +109,7 @@
108109
import java.util.function.Consumer;
109110
import java.util.function.Supplier;
110111

112+
import static org.elasticsearch.action.ResolvedIndexExpression.LocalIndexResolutionResult.CONCRETE_RESOURCE_UNAUTHORIZED;
111113
import static org.elasticsearch.action.support.ContextPreservingActionListener.wrapPreservingContext;
112114
import static org.elasticsearch.xpack.core.security.SecurityField.setting;
113115
import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.ACTION_SCOPE_AUTHORIZATION_KEYS;
@@ -529,27 +531,19 @@ private void authorizeAction(
529531
ActionListener.run(
530532
listener,
531533
l -> authzEngine.authorizeIndexAction(requestInfo, authzInfo, resolvedIndicesAsyncSupplier, projectMetadata)
532-
.addListener(
533-
wrapPreservingContext(
534-
new AuthorizationResultListener<>(
535-
result -> handleIndexActionAuthorizationResult(
536-
result,
537-
requestInfo,
538-
requestId,
539-
authzInfo,
540-
authzEngine,
541-
resolvedIndicesAsyncSupplier,
542-
projectMetadata,
543-
l
544-
),
545-
l::onFailure,
546-
requestInfo,
547-
requestId,
548-
authzInfo
549-
),
550-
threadContext
551-
)
552-
)
534+
.addListener(wrapPreservingContext(new AuthorizationResultListener<>(result -> {
535+
setIndexResolutionException(authzInfo, request, authentication, action);
536+
handleIndexActionAuthorizationResult(
537+
result,
538+
requestInfo,
539+
requestId,
540+
authzInfo,
541+
authzEngine,
542+
resolvedIndicesAsyncSupplier,
543+
projectMetadata,
544+
l
545+
);
546+
}, l::onFailure, requestInfo, requestId, authzInfo), threadContext))
553547
);
554548
}, e -> onAuthorizedResourceLoadFailure(requestId, requestInfo, authzInfo, auditTrail, listener, e)));
555549
} else {
@@ -714,6 +708,34 @@ private void handleIndexActionAuthorizationResult(
714708
}
715709
}
716710

711+
private void setIndexResolutionException(
712+
AuthorizationInfo authzInfo,
713+
TransportRequest request,
714+
Authentication authentication,
715+
String action
716+
) {
717+
if (request instanceof IndicesRequest.Replaceable replaceable) {
718+
var indexExpressions = replaceable.getResolvedIndexExpressions();
719+
if (indexExpressions != null) {
720+
indexExpressions.expressions().forEach(resolved -> {
721+
if (resolved.localExpressions().localIndexResolutionResult() == CONCRETE_RESOURCE_UNAUTHORIZED) {
722+
resolved.localExpressions()
723+
.setExceptionIfUnset(
724+
actionDenied(
725+
authentication,
726+
authzInfo,
727+
action,
728+
request,
729+
IndexAuthorizationResult.getFailureDescription(List.of(resolved.original()), restrictedIndices),
730+
null
731+
)
732+
);
733+
}
734+
});
735+
}
736+
}
737+
}
738+
717739
private void runRequestInterceptors(
718740
RequestInfo requestInfo,
719741
AuthorizationInfo authorizationInfo,

0 commit comments

Comments
 (0)