-
Notifications
You must be signed in to change notification settings - Fork 25.7k
record security exceptions in resolved index expressions #135630
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
Changes from 3 commits
eba4f92
e5e3e12
d2f5563
c236591
e7e8c16
6c302bd
359fca7
1fdb0a8
4a3ffd4
98a50b9
9d89502
f143685
6109ecb
b81d263
69e46f3
a0e594f
d99f487
dfb3081
38115ad
88f63a3
9aef86f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -104,6 +105,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; | ||
|
|
@@ -526,7 +528,9 @@ private void authorizeAction( | |
| if (e instanceof IndexNotFoundException) { | ||
| listener.onFailure(e); | ||
| } else { | ||
| listener.onFailure(actionDenied(authentication, authzInfo, action, request, e)); | ||
|
||
| final var denial = actionDenied(authentication, authzInfo, action, request, e); | ||
| setResolvedIndexException(request, denial); | ||
| listener.onFailure(denial); | ||
| } | ||
| } | ||
| ) | ||
|
|
@@ -974,6 +978,19 @@ public ElasticsearchSecurityException remoteActionDenied(Authentication authenti | |
| ); | ||
| } | ||
|
|
||
| private void setResolvedIndexException(TransportRequest request, ElasticsearchSecurityException exception) { | ||
| 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(exception); | ||
| } | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| ElasticsearchSecurityException actionDenied( | ||
| Authentication authentication, | ||
| @Nullable AuthorizationInfo authorizationInfo, | ||
|
|
@@ -1079,8 +1096,10 @@ private void handleFailure(@Nullable String context, @Nullable Exception e) { | |
| Authentication authentication = requestInfo.getAuthentication(); | ||
| String action = requestInfo.getAction(); | ||
| TransportRequest request = requestInfo.getRequest(); | ||
| final var denial = actionDenied(authentication, authzInfo, action, request, context, e); | ||
| setResolvedIndexException(request, denial); | ||
| auditTrailService.get().accessDenied(requestId, authentication, action, request, authzInfo); | ||
| failureConsumer.accept(actionDenied(authentication, authzInfo, action, request, context, e)); | ||
| failureConsumer.accept(denial); | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
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.exceptionisnullbefore set itThere was a problem hiding this comment.
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
There was a problem hiding this comment.
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.exceptionisnull. Ifthis.exceptionis already set, assert the two exceptions have the same type and error message (stacktraces are not necessarily the same).