Skip to content
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.

Sign up for GitHub

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

Add new audit handler method for action responses #63708

Merged
merged 38 commits into from
Dec 17, 2020
Merged

Add new audit handler method for action responses #63708

Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
38 commits
Select commit Hold shift + click to select a range
c710726
Remove redundant IOException
albertzaharovits Oct 14, 2020
e9f2866
Merge branch 'master' into post_authz_audit_event
albertzaharovits Oct 14, 2020
48d647e
WIP
albertzaharovits Oct 14, 2020
e3c5c3f
Merge branch 'master' into post_authz_audit_event
albertzaharovits Oct 15, 2020
9469127
SecurityActionFilterTests
albertzaharovits Oct 15, 2020
2f5e546
Strict!
albertzaharovits Oct 15, 2020
d88969b
Merge branch 'master' into post_authz_audit_event
albertzaharovits Oct 26, 2020
0ebadb9
Trash test
albertzaharovits Oct 26, 2020
f6ff433
SecurityActionFilter
albertzaharovits Oct 26, 2020
9f56238
extract requestId and authentication before running the action
albertzaharovits Oct 26, 2020
765493f
remove thrash
albertzaharovits Oct 26, 2020
cf172b8
Merge branch 'master' into post_authz_audit_event
albertzaharovits Oct 26, 2020
71790f9
Security enabled guard in SecurityServerTransport
albertzaharovits Oct 26, 2020
e25903e
AuthenticationServiceTests
albertzaharovits Oct 27, 2020
6e229a8
AuthorizationServiceTests
albertzaharovits Oct 29, 2020
43f3644
ServerTransportFilterTests
albertzaharovits Oct 29, 2020
a922aa8
SecurityActionFilterTests
albertzaharovits Oct 29, 2020
aada0ae
Merge branch 'master' into post_authz_audit_event
albertzaharovits Oct 29, 2020
655bba9
More tests
albertzaharovits Oct 29, 2020
8c03167
Checkstyle
albertzaharovits Oct 29, 2020
b514c0c
Merge branch 'master' into post_authz_audit_event
albertzaharovits Nov 4, 2020
6706993
Reuse the `User#isInternal` method in the authorization service
albertzaharovits Nov 4, 2020
ec22611
Merge branch 'master' into post_authz_audit_event
albertzaharovits Dec 7, 2020
1b5b65e
tests afte merge conflict
albertzaharovits Dec 7, 2020
e7383c5
wrong merge choice
albertzaharovits Dec 7, 2020
14425d5
Good merge choice
albertzaharovits Dec 7, 2020
a730658
Move chain.proceed down
albertzaharovits Dec 7, 2020
c6d1a74
Remove line
albertzaharovits Dec 8, 2020
93299a3
Merge branch 'master' into post_authz_audit_event
albertzaharovits Dec 9, 2020
3926ada
Checkstyle
albertzaharovits Dec 9, 2020
4d579d7
Merge branch 'master' into post_authz_audit_event
albertzaharovits Dec 14, 2020
e5ddb6e
Only intercept the action filter
albertzaharovits Dec 15, 2020
cf59a60
Checkstyle test
albertzaharovits Dec 15, 2020
7851883
Merge branch 'master' into post_authz_audit_event
albertzaharovits Dec 15, 2020
3753771
Merge branch 'master' into post_authz_audit_event
albertzaharovits Dec 15, 2020
49a2852
Nits
albertzaharovits Dec 15, 2020
6e2e882
Update x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/s…
albertzaharovits Dec 16, 2020
b81f644
Merge branch 'master' into post_authz_audit_event
elasticmachine Dec 16, 2020
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
2 changes: 1 addition & 1 deletion ...ck/plugin/ml/src/test/java/org/elasticsearch/xpack/ml/MlUpgradeModeActionFilterTests.java
View file
Open in desktop
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
5 changes: 3 additions & 2 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -519,9 +519,10 @@ auditTrailService, failureHandler, threadPool, anonymousUser, getAuthorizationEn
components.add(ipFilter.get());
DestructiveOperations destructiveOperations = new DestructiveOperations(settings, clusterService.getClusterSettings());
securityInterceptor.set(new SecurityServerTransportInterceptor(settings, threadPool, authcService.get(),
authzService, getLicenseState(), getSslService(), securityContext.get(), destructiveOperations, clusterService));
authzService, getLicenseState(), getSslService(), securityContext.get(), destructiveOperations,
clusterService));
albertzaharovits marked this conversation as resolved.
Show resolved Hide resolved

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
Open in desktop
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) {
albertzaharovits marked this conversation as resolved.
Show resolved Hide resolved
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
Open in desktop
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);
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 not technically true, some actions' (TransportMasterNodeAction) responses are intercepted on the coordinating node, as well as on the master node.

The naming and descriptions are difficult to write to convey the subset of actions that are covered.
We could also bleed some internals in here and mention the SecurityActionFilter.

Open to suggestions.

Copy link
Member

Choose a reason for hiding this comment

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

I think we could keep the previous name, i.e. actionResponse. Please let me explain: When I suggested pulling out the code from SecurityServerTransportInterceptor, it was in the context of getting auditing for API key ID in for v7.11.0. The reasons are:

  • The response in SecurityServerTransportInterceptor is a fairly low level indexing response and does not really provide extra valuable information than the one in the coordinating node. Therefore auditing on just the coordinating node is sufficient.
  • Creating API key is underlyingly a TransportReplicationAction. Unlike TransportMasterNodeAction, it does not go through the SecurityActionFilter (on the remote node). This means: on the remote node, we will not get another auditing entry from SecurityActionFilter, i.e. there will only be a single auditing entry about API key creation and it is on the coordinating node.

So my motivation was rather narrowly focused on the particular use case. The agreement, as I understand it, is to defer the auditing code in SecurityServerTransportInterceptor until the next release. Because it is not necessary for API key auditing in this release and if it is not in use, it's better to be left out. But we also agreed that it could still be useful when we expand the usage scenario and this is subject to further discussion.

IIUC, your intention of changing the method name is for the possibility to have two different audit event types. I should have been clear that it was not my proposal. I consider whether auditing inside SecurityServerTransportInterceptor an implementation detail, which we chose to opt out for initial release, but could choose to add it later as an enhancement. For verbosity and duplication control, I'd prefer to solve it in a different way other than splitting it into different audit event types. I hope this makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the detailed explanation Yang, but I would like to focus the conversation on the scope of this PR only. We should consider the broader picture, as you describe, but ultimately this PR is what gets merged as a unit, and what we merge in the future is subject to debate still.

If we merge this PR, which adds the audit hook for the responses of actions intercepted by the SecurityActionFilter only, we add to the interface of the LoggingAuditTrail .
When we later merge the code to allow intercepting the responses of the other actions, we have the options to:

  • reuse the same LoggingAuditTrail hook method
  • rename the existing hook method, or
  • add a new audit response hook method.
    (or any mix of them, really, if we feel wild)

Because this is a private interface, I don't believe we have to name it in anticipation of its use. We should name it for the purpose that it serves right now, as we can always tinker with it later. For this reason, I would not favor the actionResponse name.

Copy link
Member

Choose a reason for hiding this comment

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

Just trying to brainstorm here: How about calling this one actionResponse and the other one transportResponse?
As you said, this is a private interface, so we don't have to struggle too much with its name now. So please feel free to use whatever works for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this could work. I'll keep this in mind when we'll iterate on the interface.

}
15 changes: 15 additions & 0 deletions ...ugin/security/src/main/java/org/elasticsearch/xpack/security/audit/AuditTrailService.java
View file
Open in desktop
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
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,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.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.AuthenticationToken;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo;
Expand Down Expand Up @@ -731,6 +732,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 class LogEntryBuilder {

private final StringMapMessage logEntry;
Expand Down
13 changes: 4 additions & 9 deletions ...n/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java
View file
Open in desktop
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;
ywangd marked this conversation as resolved.
Show resolved Hide resolved
}
}

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