Skip to content

Commit

Permalink
Decouple AuditTrailService from AuditTrail (elastic#53450)
Browse files Browse the repository at this point in the history
The AuditTrailService has historically been an AuditTrail itself, acting
as a composite of the configured audit trails. This commit removes that
interface from the service and instead builds a composite delegating
implementation internally. The service now has a single get() method to
get an AuditTrail implementation which may be called. If auditing is not
allowed by the license, an empty noop version is returned.
  • Loading branch information
rjernst authored Mar 18, 2020
1 parent f26ccb2 commit 9467dbf
Show file tree
Hide file tree
Showing 13 changed files with 287 additions and 187 deletions.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ public class AuthenticationService {
private static final Logger logger = LogManager.getLogger(AuthenticationService.class);

private final Realms realms;
private final AuditTrail auditTrail;
private final AuditTrailService auditTrailService;
private final AuthenticationFailureHandler failureHandler;
private final ThreadContext threadContext;
private final String nodeName;
Expand All @@ -90,12 +90,12 @@ public class AuthenticationService {
private final boolean isAnonymousUserEnabled;
private final AuthenticationContextSerializer authenticationSerializer;

public AuthenticationService(Settings settings, Realms realms, AuditTrailService auditTrail,
public AuthenticationService(Settings settings, Realms realms, AuditTrailService auditTrailService,
AuthenticationFailureHandler failureHandler, ThreadPool threadPool,
AnonymousUser anonymousUser, TokenService tokenService, ApiKeyService apiKeyService) {
this.nodeName = Node.NODE_NAME_SETTING.get(settings);
this.realms = realms;
this.auditTrail = auditTrail;
this.auditTrailService = auditTrailService;
this.failureHandler = failureHandler;
this.threadContext = threadPool.getThreadContext();
this.anonymousUser = anonymousUser;
Expand Down Expand Up @@ -274,16 +274,17 @@ class Authenticator {
private AuthenticationResult authenticationResult = null;

Authenticator(RestRequest request, boolean fallbackToAnonymous, ActionListener<Authentication> listener) {
this(new AuditableRestRequest(auditTrail, failureHandler, threadContext, request), null, fallbackToAnonymous, listener);
this(new AuditableRestRequest(auditTrailService.get(), failureHandler, threadContext, request),
null, fallbackToAnonymous, listener);
}

Authenticator(String action, TransportMessage message, boolean fallbackToAnonymous, ActionListener<Authentication> listener) {
this(new AuditableTransportRequest(auditTrail, failureHandler, threadContext, action, message),
this(new AuditableTransportRequest(auditTrailService.get(), failureHandler, threadContext, action, message),
null, fallbackToAnonymous, listener);
}

Authenticator(String action, TransportMessage message, User fallbackUser, ActionListener<Authentication> listener) {
this(new AuditableTransportRequest(auditTrail, failureHandler, threadContext, action, message),
this(new AuditableTransportRequest(auditTrailService.get(), failureHandler, threadContext, action, message),
Objects.requireNonNull(fallbackUser, "Fallback user cannot be null"), false, listener);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
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;
import org.elasticsearch.xpack.security.audit.AuditUtil;
import org.elasticsearch.xpack.security.authc.ApiKeyService;
Expand Down Expand Up @@ -101,7 +102,7 @@ public class AuthorizationService {

private final Settings settings;
private final ClusterService clusterService;
private final AuditTrailService auditTrail;
private final AuditTrailService auditTrailService;
private final IndicesAndAliasesResolver indicesAndAliasesResolver;
private final AuthenticationFailureHandler authcFailureHandler;
private final ThreadContext threadContext;
Expand All @@ -114,12 +115,12 @@ public class AuthorizationService {
private final boolean anonymousAuthzExceptionEnabled;

public AuthorizationService(Settings settings, CompositeRolesStore rolesStore, ClusterService clusterService,
AuditTrailService auditTrail, AuthenticationFailureHandler authcFailureHandler,
AuditTrailService auditTrailService, AuthenticationFailureHandler authcFailureHandler,
ThreadPool threadPool, AnonymousUser anonymousUser, @Nullable AuthorizationEngine authorizationEngine,
Set<RequestInterceptor> requestInterceptors, XPackLicenseState licenseState,
IndexNameExpressionResolver resolver) {
this.clusterService = clusterService;
this.auditTrail = auditTrail;
this.auditTrailService = auditTrailService;
this.indicesAndAliasesResolver = new IndicesAndAliasesResolver(settings, clusterService, resolver);
this.authcFailureHandler = authcFailureHandler;
this.threadContext = threadPool.getThreadContext();
Expand Down Expand Up @@ -172,7 +173,7 @@ public void authorize(final Authentication authentication, final String action,
if (isInternalUser(authentication.getUser()) != false) {
auditId = AuditUtil.getOrGenerateRequestId(threadContext);
} else {
auditTrail.tamperedRequest(null, authentication.getUser(), action, originalRequest);
auditTrailService.get().tamperedRequest(null, authentication.getUser(), action, originalRequest);
final String message = "Attempt to authorize action [" + action + "] for [" + authentication.getUser().principal()
+ "] without an existing request-id";
assert false : message;
Expand Down Expand Up @@ -204,6 +205,7 @@ private void maybeAuthorizeRunAs(final RequestInfo requestInfo, final String req
final TransportRequest request = requestInfo.getRequest();
final String action = requestInfo.getAction();
final boolean isRunAs = authentication.getUser().isRunAs();
final AuditTrail auditTrail = auditTrailService.get();
if (isRunAs) {
ActionListener<AuthorizationResult> runAsListener = wrapPreservingContext(ActionListener.wrap(result -> {
if (result.isGranted()) {
Expand Down Expand Up @@ -236,6 +238,7 @@ private void authorizeAction(final RequestInfo requestInfo, final String request
final TransportRequest request = requestInfo.getRequest();
final String action = requestInfo.getAction();
final AuthorizationEngine authzEngine = getAuthorizationEngine(authentication);
final AuditTrail auditTrail = auditTrailService.get();
if (ClusterPrivilegeResolver.isClusterAction(action)) {
final ActionListener<AuthorizationResult> clusterAuthzListener =
wrapPreservingContext(new AuthorizationResultListener<>(result -> {
Expand Down Expand Up @@ -373,6 +376,7 @@ private AuthorizationEngine getAuthorizationEngineForUser(final User user) {

private void authorizeSystemUser(final Authentication authentication, final String action, final String requestId,
final TransportRequest request, final ActionListener<Void> listener) {
final AuditTrail auditTrail = auditTrailService.get();
if (SystemUser.isAuthorized(action)) {
putTransientIfNonExisting(AuthorizationServiceField.INDICES_PERMISSIONS_KEY, IndicesAccessControl.ALLOW_ALL);
putTransientIfNonExisting(AUTHORIZATION_INFO_KEY, SYSTEM_AUTHZ_INFO);
Expand All @@ -394,6 +398,7 @@ private TransportRequest maybeUnwrapRequest(Authentication authentication, Trans
request = TransportActionProxy.unwrapRequest(originalRequest);
final boolean isOriginalRequestProxyRequest = TransportActionProxy.isProxyRequest(originalRequest);
final boolean isProxyAction = TransportActionProxy.isProxyAction(action);
final AuditTrail auditTrail = auditTrailService.get();
if (isProxyAction && isOriginalRequestProxyRequest == false) {
IllegalStateException cause = new IllegalStateException("originalRequest is not a proxy request: [" + originalRequest +
"] but action: [" + action + "] is a proxy action");
Expand Down Expand Up @@ -451,6 +456,7 @@ private void authorizeBulkItems(RequestInfo requestInfo, AuthorizationInfo authz
final Map<String, String> resolvedIndexNames = new HashMap<>();
// Maps action -> resolved indices set
final Map<String, Set<String>> actionToIndicesMap = new HashMap<>();
final AuditTrail auditTrail = auditTrailService.get();

authorizedIndicesSupplier.getAsync(ActionListener.wrap(authorizedIndices -> {
resolvedIndicesAsyncSupplier.getAsync(ActionListener.wrap(overallResolvedIndices -> {
Expand Down Expand Up @@ -611,8 +617,8 @@ private AuthorizationResultListener(Consumer<T> responseConsumer, Consumer<Excep
public void onResponse(T result) {
if (result.isGranted()) {
if (result.isAuditable()) {
auditTrail.accessGranted(requestId, requestInfo.getAuthentication(), requestInfo.getAction(), requestInfo.getRequest(),
authzInfo);
auditTrailService.get().accessGranted(requestId, requestInfo.getAuthentication(),
requestInfo.getAction(), requestInfo.getRequest(), authzInfo);
}
try {
responseConsumer.accept(result);
Expand All @@ -631,8 +637,8 @@ public void onFailure(Exception e) {

private void handleFailure(boolean audit, @Nullable Exception e) {
if (audit) {
auditTrail.accessDenied(requestId, requestInfo.getAuthentication(), requestInfo.getAction(), requestInfo.getRequest(),
authzInfo);
auditTrailService.get().accessDenied(requestId, requestInfo.getAuthentication(), requestInfo.getAction(),
requestInfo.getRequest(), authzInfo);
}
failureConsumer.accept(denialException(requestInfo.getAuthentication(), requestInfo.getAction(), e));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ static void ensureAuthenticatedUserIsSame(Authentication original, Authenticatio

final boolean sameUser = samePrincipal && sameRealmType;
if (sameUser == false) {
auditTrailService.accessDenied(requestId, current, action, request, authorizationInfo);
auditTrailService.get().accessDenied(requestId, current, action, request, authorizationInfo);
throw new SearchContextMissingException(id);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.security.support.Exceptions;
import org.elasticsearch.xpack.security.audit.AuditTrail;
import org.elasticsearch.xpack.security.audit.AuditTrailService;
import org.elasticsearch.xpack.security.audit.AuditUtil;

Expand Down Expand Up @@ -48,6 +49,7 @@ public void intercept(RequestInfo requestInfo, AuthorizationEngine authorization
if (requestInfo.getRequest() instanceof IndicesAliasesRequest) {
final IndicesAliasesRequest request = (IndicesAliasesRequest) requestInfo.getRequest();
final XPackLicenseState frozenLicenseState = licenseState.copyCurrentLicenseState();
final AuditTrail auditTrail = auditTrailService.get();
if (frozenLicenseState.isAuthAllowed()) {
if (frozenLicenseState.isDocumentAndFieldLevelSecurityAllowed()) {
IndicesAccessControl indicesAccessControl =
Expand Down Expand Up @@ -89,7 +91,7 @@ public void intercept(RequestInfo requestInfo, AuthorizationEngine authorization
// do not audit success again
listener.onResponse(null);
} else {
auditTrailService.accessDenied(AuditUtil.extractRequestId(threadContext), requestInfo.getAuthentication(),
auditTrail.accessDenied(AuditUtil.extractRequestId(threadContext), requestInfo.getAuthentication(),
requestInfo.getAction(), request, authorizationInfo);
listener.onFailure(Exceptions.authorizationError("Adding an alias is not allowed when the alias " +
"has more permissions than any of the indices"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.xpack.core.security.authz.AuthorizationServiceField;
import org.elasticsearch.xpack.core.security.authz.accesscontrol.IndicesAccessControl;
import org.elasticsearch.xpack.core.security.support.Exceptions;
import org.elasticsearch.xpack.security.audit.AuditTrail;
import org.elasticsearch.xpack.security.audit.AuditTrailService;

import java.util.Collections;
Expand All @@ -44,6 +45,7 @@ public void intercept(RequestInfo requestInfo, AuthorizationEngine authorization
if (requestInfo.getRequest() instanceof ResizeRequest) {
final ResizeRequest request = (ResizeRequest) requestInfo.getRequest();
final XPackLicenseState frozenLicenseState = licenseState.copyCurrentLicenseState();
final AuditTrail auditTrail = auditTrailService.get();
if (frozenLicenseState.isAuthAllowed()) {
if (frozenLicenseState.isDocumentAndFieldLevelSecurityAllowed()) {
IndicesAccessControl indicesAccessControl =
Expand All @@ -68,7 +70,7 @@ public void intercept(RequestInfo requestInfo, AuthorizationEngine authorization
listener.onResponse(null);
} else {
if (authzResult.isAuditable()) {
auditTrailService.accessDenied(extractRequestId(threadContext), requestInfo.getAuthentication(),
auditTrail.accessDenied(extractRequestId(threadContext), requestInfo.getAuthentication(),
requestInfo.getAction(), request, authorizationInfo);
}
listener.onFailure(Exceptions.authorizationError("Resizing an index is not allowed when the target index " +
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import org.elasticsearch.common.transport.TransportAddress;
import org.elasticsearch.license.XPackLicenseState;
import org.elasticsearch.transport.TransportSettings;
import org.elasticsearch.xpack.security.audit.AuditTrail;
import org.elasticsearch.xpack.security.audit.AuditTrailService;

import java.net.InetSocketAddress;
Expand Down Expand Up @@ -96,7 +97,7 @@ public IpFilterRuleType ruleType() {

private static final Logger logger = LogManager.getLogger(IPFilter.class);

private final AuditTrailService auditTrail;
private final AuditTrailService auditTrailService;
private final XPackLicenseState licenseState;
private final boolean alwaysAllowBoundAddresses;

Expand All @@ -114,9 +115,9 @@ public IpFilterRuleType ruleType() {
private final Map<String, List<String>> profileAllowRules = Collections.synchronizedMap(new HashMap<>());
private final Map<String, List<String>> profileDenyRules = Collections.synchronizedMap(new HashMap<>());

public IPFilter(final Settings settings, AuditTrailService auditTrail, ClusterSettings clusterSettings,
public IPFilter(final Settings settings, AuditTrailService auditTrailService, ClusterSettings clusterSettings,
XPackLicenseState licenseState) {
this.auditTrail = auditTrail;
this.auditTrailService = auditTrailService;
this.licenseState = licenseState;
this.alwaysAllowBoundAddresses = ALLOW_BOUND_ADDRESSES_SETTING.get(settings);
httpDenyFilter = HTTP_FILTER_DENY_SETTING.get(settings);
Expand Down Expand Up @@ -205,6 +206,7 @@ public boolean accept(String profile, InetSocketAddress peerAddress) {
return true;
}

AuditTrail auditTrail = auditTrailService.get();
for (SecurityIpFilterRule rule : rules.get(profile)) {
if (rule.matches(peerAddress)) {
boolean isAllowed = rule.ruleType() == IpFilterRuleType.ACCEPT;
Expand Down
Loading

0 comments on commit 9467dbf

Please sign in to comment.