Skip to content

Commit

Permalink
Add new audit handler for action responses (#63708)
Browse files Browse the repository at this point in the history 
This adds a new method to the AuditTrail that intercepts the
responses of transport-level actions. This new method is unlike all
the other existing audit methods because it is called after the
action has been run (so that it has access to the response).
After careful deliberation, the new method is called for the
responses of actions that are intercepted by the
`SecurityActionFilter` only, and not by the transport filter.

In order to facilitate the "linking" of the new audit event with the
other existing events, the audit method receives the requestId
as well as the authentication as arguments (in addition to the
request itself and the response).

This is labeled non-issue because it is only the foundation
upon which later features that actually print out (some) responses
can be built upon.

Related #63221
  • Loading branch information
@albertzaharovits
albertzaharovits authored Dec 17, 2020
1 parent 746618b commit d967c75
Show file tree
Hide file tree
Showing 9 changed files with 583 additions and 145 deletions.
2 changes: 1 addition & 1 deletion ...ck/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MlUpgradeModeActionFilterTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ public void testApply_ActionAllowedInUpgradeMode() {

public void testOrder_UpgradeFilterIsExecutedAfterSecurityFilter() {
MlUpgradeModeActionFilter upgradeModeFilter = new MlUpgradeModeActionFilter(clusterService);
SecurityActionFilter securityFilter = new SecurityActionFilter(null, null, null, mock(ThreadPool.class), null, null);
SecurityActionFilter securityFilter = new SecurityActionFilter(null, null, null, null, mock(ThreadPool.class), null, null);

ActionFilter[] actionFiltersInOrderOfExecution = new ActionFilters(Sets.newHashSet(upgradeModeFilter, securityFilter)).filters();
assertThat(actionFiltersInOrderOfExecution, is(arrayContaining(securityFilter, upgradeModeFilter)));
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ auditTrailService, failureHandler, threadPool, anonymousUser, getAuthorizationEn
securityInterceptor.set(new SecurityServerTransportInterceptor(settings, threadPool, authcService.get(),
authzService, getLicenseState(), getSslService(), securityContext.get(), destructiveOperations, clusterService));

securityActionFilter.set(new SecurityActionFilter(authcService.get(), authzService, getLicenseState(),
securityActionFilter.set(new SecurityActionFilter(authcService.get(), authzService, auditTrailService, getLicenseState(),
threadPool, securityContext.get(), destructiveOperations));

components.add(new SecurityUsageServices(realms, allRolesStore, nativeRoleMappingStore, ipFilter.get()));
Expand Down
47 changes: 25 additions & 22 deletions ...ty/src/main/java/org/elasticsearch/xpack/security/action/filter/SecurityActionFilter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import org.elasticsearch.action.support.ActionFilterChain;
import org.elasticsearch.action.support.ContextPreservingActionListener;
import org.elasticsearch.action.support.DestructiveOperations;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.util.concurrent.ThreadContext;
import org.elasticsearch.license.LicenseUtils;
import org.elasticsearch.license.XPackLicenseState;
Expand All @@ -33,11 +34,12 @@
import org.elasticsearch.xpack.core.security.support.Automatons;
import org.elasticsearch.xpack.core.security.user.SystemUser;
import org.elasticsearch.xpack.security.action.SecurityActionMapper;
import org.elasticsearch.xpack.security.audit.AuditTrailService;
import org.elasticsearch.xpack.security.audit.AuditUtil;
import org.elasticsearch.xpack.security.authc.AuthenticationService;
import org.elasticsearch.xpack.security.authz.AuthorizationService;
import org.elasticsearch.xpack.security.authz.AuthorizationUtils;

import java.io.IOException;
import java.util.function.Predicate;

public class SecurityActionFilter implements ActionFilter {
Expand All @@ -48,17 +50,19 @@ public class SecurityActionFilter implements ActionFilter {

private final AuthenticationService authcService;
private final AuthorizationService authzService;
private final AuditTrailService auditTrailService;
private final SecurityActionMapper actionMapper = new SecurityActionMapper();
private final XPackLicenseState licenseState;
private final ThreadContext threadContext;
private final SecurityContext securityContext;
private final DestructiveOperations destructiveOperations;

public SecurityActionFilter(AuthenticationService authcService, AuthorizationService authzService,
XPackLicenseState licenseState, ThreadPool threadPool,
AuditTrailService auditTrailService, XPackLicenseState licenseState, ThreadPool threadPool,
SecurityContext securityContext, DestructiveOperations destructiveOperations) {
this.authcService = authcService;
this.authzService = authzService;
this.auditTrailService = auditTrailService;
this.licenseState = licenseState;
this.threadContext = threadPool.getThreadContext();
this.securityContext = securityContext;
Expand All @@ -83,29 +87,19 @@ public <Request extends ActionRequest, Response extends ActionResponse> void app
if (licenseState.isSecurityEnabled()) {
final ActionListener<Response> contextPreservingListener =
ContextPreservingActionListener.wrapPreservingContext(listener, threadContext);
ActionListener<Void> authenticatedListener = ActionListener.wrap(
(aVoid) -> chain.proceed(task, action, request, contextPreservingListener), contextPreservingListener::onFailure);
final boolean useSystemUser = AuthorizationUtils.shouldReplaceUserWithSystem(threadContext, action);
try {
if (useSystemUser) {
securityContext.executeAsUser(SystemUser.INSTANCE, (original) -> {
try {
applyInternal(action, request, authenticatedListener);
} catch (IOException e) {
listener.onFailure(e);
}
applyInternal(task, chain, action, request, contextPreservingListener);
}, Version.CURRENT);
} else if (AuthorizationUtils.shouldSetUserBasedOnActionOrigin(threadContext)) {
AuthorizationUtils.switchUserBasedOnActionOriginAndExecute(threadContext, securityContext, (original) -> {
try {
applyInternal(action, request, authenticatedListener);
} catch (IOException e) {
listener.onFailure(e);
}
applyInternal(task, chain, action, request, contextPreservingListener);
});
} else {
try (ThreadContext.StoredContext ignore = threadContext.newStoredContext(true)) {
applyInternal(action, request, authenticatedListener);
applyInternal(task, chain, action, request, contextPreservingListener);
}
}
} catch (Exception e) {
Expand All @@ -130,13 +124,13 @@ public int order() {
return Integer.MIN_VALUE;
}

private <Request extends ActionRequest> void applyInternal(String action, Request request,
ActionListener<Void> listener) throws IOException {
private <Request extends ActionRequest, Response extends ActionResponse> void applyInternal(Task task,
ActionFilterChain<Request, Response> chain, String action, Request request, ActionListener<Response> listener) {
if (CloseIndexAction.NAME.equals(action) || OpenIndexAction.NAME.equals(action) || DeleteIndexAction.NAME.equals(action)) {
IndicesRequest indicesRequest = (IndicesRequest) request;
try {
destructiveOperations.failDestructive(indicesRequest.indices());
} catch(IllegalArgumentException e) {
} catch (IllegalArgumentException e) {
listener.onFailure(e);
return;
}
Expand All @@ -156,7 +150,17 @@ it to the action without an associated user (not via REST or transport - this is
authcService.authenticate(securityAction, request, SystemUser.INSTANCE,
ActionListener.wrap((authc) -> {
if (authc != null) {
authorizeRequest(authc, securityAction, request, listener);
final String requestId = AuditUtil.extractRequestId(threadContext);
assert Strings.hasText(requestId);
authorizeRequest(authc, securityAction, request, ActionListener.delegateFailure(listener,
(ignore, aVoid) -> {
chain.proceed(task, action, request, ActionListener.delegateFailure(listener,
(ignore2, response) -> {
auditTrailService.get().coordinatingActionResponse(requestId, authc, action, request,
response);
listener.onResponse(response);
}));
}));
} else if (licenseState.isSecurityEnabled() == false) {
listener.onResponse(null);
} else {
Expand All @@ -166,12 +170,11 @@ it to the action without an associated user (not via REST or transport - this is
}

private <Request extends ActionRequest> void authorizeRequest(Authentication authentication, String securityAction, Request request,
ActionListener<Void> listener) {
ActionListener<Void> listener) {
if (authentication == null) {
listener.onFailure(new IllegalArgumentException("authentication must be non null for authorization"));
} else {
authzService.authorize(authentication, securityAction, request, ActionListener.wrap(ignore -> listener.onResponse(null),
listener::onFailure));
authzService.authorize(authentication, securityAction, request, listener);
}
}
}
6 changes: 6 additions & 0 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrail.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import org.elasticsearch.common.transport.TransportAddress;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.transport.TransportResponse;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.AuthenticationToken;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo;
Expand Down Expand Up @@ -81,4 +82,9 @@ void runAsDenied(String requestId, Authentication authentication, RestRequest re
void explicitIndexAccessEvent(String requestId, AuditLevel eventType, Authentication authentication, String action, String indices,
String requestName, TransportAddress remoteAddress, AuthorizationInfo authorizationInfo);

// this is the only audit method that is called *after* the action executed, when the response is available
// it is however *only called for coordinating actions*, which are the actions that a client invokes as opposed to
// the actions that a node invokes in order to service a client request
void coordinatingActionResponse(String requestId, Authentication authentication, String action, TransportRequest transportRequest,
TransportResponse transportResponse);
}
15 changes: 15 additions & 0 deletions ...ugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrailService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import org.elasticsearch.license.XPackLicenseState.Feature;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.transport.TransportResponse;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.AuthenticationToken;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo;
Expand Down Expand Up @@ -147,6 +148,11 @@ public void runAsDenied(String requestId, Authentication authentication, RestReq
public void explicitIndexAccessEvent(String requestId, AuditLevel eventType, Authentication authentication,
String action, String indices, String requestName, TransportAddress remoteAddress,
AuthorizationInfo authorizationInfo) {}

@Override
public void coordinatingActionResponse(String requestId, Authentication authentication, String action,
TransportRequest transportRequest,
TransportResponse transportResponse) { }
}

private static class CompositeAuditTrail implements AuditTrail {
Expand Down Expand Up @@ -254,6 +260,15 @@ public void accessDenied(String requestId, Authentication authentication, String
}
}

@Override
public void coordinatingActionResponse(String requestId, Authentication authentication, String action,
TransportRequest transportRequest,
TransportResponse transportResponse) {
for (AuditTrail auditTrail : auditTrails) {
auditTrail.coordinatingActionResponse(requestId, authentication, action, transportRequest, transportResponse);
}
}

@Override
public void tamperedRequest(String requestId, RestRequest request) {
for (AuditTrail auditTrail : auditTrails) {
Expand Down
8 changes: 8 additions & 0 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/audit/logfile/LoggingAuditTrail.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import org.elasticsearch.tasks.Task;
import org.elasticsearch.threadpool.ThreadPool;
import org.elasticsearch.transport.TransportRequest;
import org.elasticsearch.transport.TransportResponse;
import org.elasticsearch.xpack.core.security.action.CreateApiKeyAction;
import org.elasticsearch.xpack.core.security.action.CreateApiKeyRequest;
import org.elasticsearch.xpack.core.security.action.GrantApiKeyAction;
Expand Down Expand Up @@ -815,6 +816,13 @@ public void runAsDenied(String requestId, Authentication authentication, RestReq
}
}

@Override
public void coordinatingActionResponse(String requestId, Authentication authentication, String action,
TransportRequest transportRequest,
TransportResponse transportResponse) {
// not implemented yet
}

private LogEntryBuilder securityChangeLogEntryBuilder(String requestId) {
return new LogEntryBuilder(false)
.with(EVENT_TYPE_FIELD_NAME, SECURITY_CHANGE_ORIGIN_FIELD_VALUE)
Expand Down
13 changes: 4 additions & 9 deletions ...n/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,11 +61,8 @@
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
import org.elasticsearch.xpack.core.security.user.AsyncSearchUser;
import org.elasticsearch.xpack.core.security.user.SystemUser;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.core.security.user.XPackSecurityUser;
import org.elasticsearch.xpack.core.security.user.XPackUser;
import org.elasticsearch.xpack.security.audit.AuditLevel;
import org.elasticsearch.xpack.security.audit.AuditTrail;
import org.elasticsearch.xpack.security.audit.AuditTrailService;
Expand Down Expand Up @@ -96,6 +93,7 @@
import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.INDICES_PERMISSIONS_KEY;
import static org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField.ORIGINATING_ACTION_KEY;
import static org.elasticsearch.xpack.core.security.support.Exceptions.authorizationError;
import static org.elasticsearch.xpack.core.security.user.User.isInternal;
import static org.elasticsearch.xpack.security.audit.logfile.LoggingAuditTrail.PRINCIPAL_ROLES_FIELD_NAME;

public class AuthorizationService {
Expand Down Expand Up @@ -191,14 +189,15 @@ public void authorize(final Authentication authentication, final String action,
if (auditId == null) {
// We would like to assert that there is an existing request-id, but if this is a system action, then that might not be
// true because the request-id is generated during authentication
if (isInternalUser(authentication.getUser()) != false) {
if (isInternal(authentication.getUser())) {
auditId = AuditUtil.getOrGenerateRequestId(threadContext);
} else {
auditTrailService.get().tamperedRequest(null, authentication, action, originalRequest);
final String message = "Attempt to authorize action [" + action + "] for [" + authentication.getUser().principal()
+ "] without an existing request-id";
assert false : message;
listener.onFailure(new ElasticsearchSecurityException(message));
return;
}
}

Expand Down Expand Up @@ -398,7 +397,7 @@ AuthorizationEngine getAuthorizationEngine(final Authentication authentication)
private AuthorizationEngine getAuthorizationEngineForUser(final User user) {
if (rbacEngine != authorizationEngine && licenseState.isSecurityEnabled() &&
licenseState.checkFeature(Feature.SECURITY_AUTHORIZATION_ENGINE)) {
if (ClientReservedRealm.isReserved(user.principal(), settings) || isInternalUser(user)) {
if (ClientReservedRealm.isReserved(user.principal(), settings) || isInternal(user)) {
return rbacEngine;
} else {
return authorizationEngine;
Expand Down Expand Up @@ -449,10 +448,6 @@ private TransportRequest maybeUnwrapRequest(Authentication authentication, Trans
return request;
}

private boolean isInternalUser(User user) {
return SystemUser.is(user) || XPackUser.is(user) || XPackSecurityUser.is(user) || AsyncSearchUser.is(user);
}

private void authorizeRunAs(final RequestInfo requestInfo, final AuthorizationInfo authzInfo,
final ActionListener<AuthorizationResult> listener) {
final Authentication authentication = requestInfo.getAuthentication();
Expand Down
Loading

0 comments on commit d967c75

Please sign in to comment.