From 9665a631932d648688b18b141828313fe86e2e56 Mon Sep 17 00:00:00 2001 From: Yang Wang Date: Mon, 18 Oct 2021 17:25:15 +1100 Subject: [PATCH] Better logging and internal user handling for operator privileges (#79331) This PR improves logging situation for operator privilegs. The loggings are now more informational and also do not rely on the tracing level. Internal user handling is also improved by skipping them more consistently when marking and checking for operator users. The existing code performs unnecessary checks for internal actions such as leader/follower checks. It is technically a bug because threadContext is prepared differently for internal actions and does not have the operatorUser marking. However, the bug currently does not manifest itself since internal actions are not protected by operator privileges. --- .../operator/OperatorPrivilegesIT.java | 1 + .../security/authz/AuthorizationService.java | 6 +- .../operator/FileOperatorUsersStore.java | 28 ++--- .../security/operator/OperatorPrivileges.java | 34 +++-- .../authz/AuthorizationServiceTests.java | 4 +- .../operator/FileOperatorUsersStoreTests.java | 68 ++++++---- .../operator/OperatorPrivilegesTests.java | 118 +++++++++++++++--- 7 files changed, 193 insertions(+), 66 deletions(-) diff --git a/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java b/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java index db216dd0d8a51..e522948827dc9 100644 --- a/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java +++ b/x-pack/plugin/security/qa/operator-privileges-tests/src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java @@ -68,6 +68,7 @@ public void testNonOperatorSuperuserWillFailToCallOperatorOnlyApiWhenOperatorPri ); assertThat(responseException.getResponse().getStatusLine().getStatusCode(), equalTo(403)); assertThat(responseException.getMessage(), containsString("Operator privileges are required for action")); + assertThat(responseException.getMessage(), containsString("because it requires operator privileges")); } public void testOperatorUserWillSucceedToCallOperatorOnlyApi() throws IOException { diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java index f0dacb97fd90a..2d8d47566fa29 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java @@ -286,9 +286,11 @@ private void checkOperatorPrivileges(Authentication authentication, String actio throws ElasticsearchSecurityException { // Check operator privileges // TODO: audit? - final ElasticsearchSecurityException operatorException = operatorPrivilegesService.check(action, originalRequest, threadContext); + final ElasticsearchSecurityException operatorException = + operatorPrivilegesService.check(authentication, action, originalRequest, threadContext); if (operatorException != null) { - throw denialException(authentication, action, originalRequest, operatorException); + throw denialException(authentication, action, originalRequest, + "because it requires operator privileges", operatorException); } operatorPrivilegesService.maybeInterceptRequest(threadContext, originalRequest); } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java index 51373ddde21bf..431a798399263 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java @@ -29,7 +29,6 @@ import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings; import org.elasticsearch.xpack.core.security.authc.file.FileRealmSettings; -import org.elasticsearch.xpack.core.security.user.User; import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm; import java.io.IOException; @@ -65,13 +64,6 @@ public FileOperatorUsersStore(Environment env, ResourceWatcherService watcherSer } public boolean isOperatorUser(Authentication authentication) { - if (authentication.getUser().isRunAs()) { - return false; - } else if (User.isInternal(authentication.getUser())) { - // Internal user are considered operator users - return true; - } - // Other than realm name, other criteria must always be an exact match for the user to be an operator. // Realm name of a descriptor can be null. When it is null, it is ignored for comparison. // If not null, it will be compared exactly as well. @@ -79,10 +71,13 @@ public boolean isOperatorUser(Authentication authentication) { // not matter what the name is. return operatorUsersDescriptor.groups.stream().anyMatch(group -> { final Authentication.RealmRef realm = authentication.getSourceRealm(); - return group.usernames.contains(authentication.getUser().principal()) + final boolean match = group.usernames.contains(authentication.getUser().principal()) && group.authenticationType == authentication.getAuthenticationType() && realm.getType().equals(group.realmType) && (group.realmName == null || group.realmName.equals(realm.getName())); + logger.trace("Matching user [{}] against operator rule [{}] is [{}]", + authentication.getUser(), group, match); + return match; }); } @@ -218,7 +213,13 @@ public static OperatorUsersDescriptor parseFile(Path file, Logger logger) { } else { logger.debug("Reading operator users file [{}]", file.toAbsolutePath()); try (InputStream in = Files.newInputStream(file, StandardOpenOption.READ)) { - return parseConfig(in); + final OperatorUsersDescriptor operatorUsersDescriptor = parseConfig(in); + logger.info("parsed [{}] group(s) with a total of [{}] operator user(s) from file [{}]", + operatorUsersDescriptor.groups.size(), + operatorUsersDescriptor.groups.stream().mapToLong(g -> g.usernames.size()).sum(), + file.toAbsolutePath()); + logger.debug("operator user descriptor: [{}]", operatorUsersDescriptor); + return operatorUsersDescriptor; } catch (IOException | RuntimeException e) { logger.error(new ParameterizedMessage("Failed to parse operator users file [{}].", file), e); throw new ElasticsearchParseException("Error parsing operator users file [{}]", e, file.toAbsolutePath()); @@ -226,11 +227,10 @@ public static OperatorUsersDescriptor parseFile(Path file, Logger logger) { } } - public static OperatorUsersDescriptor parseConfig(InputStream in) throws IOException { + // package method for testing + static OperatorUsersDescriptor parseConfig(InputStream in) throws IOException { try (XContentParser parser = yamlParser(in)) { - final OperatorUsersDescriptor operatorUsersDescriptor = OPERATOR_USER_PARSER.parse(parser, null); - logger.trace("Parsed: [{}]", operatorUsersDescriptor); - return operatorUsersDescriptor; + return OPERATOR_USER_PARSER.parse(parser, null); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java index bb8967d78c58c..203e94b8d0f56 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java @@ -17,6 +17,7 @@ import org.elasticsearch.transport.TransportRequest; import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.AuthenticationField; +import org.elasticsearch.xpack.core.security.user.User; public class OperatorPrivileges { @@ -36,10 +37,14 @@ public interface OperatorPrivilegesService { * Check whether the user is an operator and whether the request is an operator-only. * @return An exception if user is an non-operator and the request is operator-only. Otherwise returns null. */ - ElasticsearchSecurityException check(String action, TransportRequest request, ThreadContext threadContext); + ElasticsearchSecurityException check( + Authentication authentication, String action, TransportRequest request, ThreadContext threadContext); /** - * Check the threadContext to see whether the current authenticating user is an operator user + * When operator privileges are enabled, certain requests needs to be configured in a specific way + * so that they respect operator only settings. For an example, the restore snapshot request + * should not restore operator only states from the snapshot. + * This method is where that requests are configured when necessary. */ void maybeInterceptRequest(ThreadContext threadContext, TransportRequest request); } @@ -63,20 +68,32 @@ public void maybeMarkOperatorUser(Authentication authentication, ThreadContext t if (false == shouldProcess()) { return; } - if (fileOperatorUsersStore.isOperatorUser(authentication)) { - logger.trace("User [{}] is an operator", authentication.getUser().principal()); + // Let internal users pass, they are exempt from marking and checking + if (User.isInternal(authentication.getUser())) { + return; + } + // An operator user must not be a run_as user, it also must be recognised by the operatorUserStore + if (false == authentication.getUser().isRunAs() && fileOperatorUsersStore.isOperatorUser(authentication)) { + logger.debug("Marking user [{}] as an operator", authentication.getUser()); threadContext.putHeader(AuthenticationField.PRIVILEGE_CATEGORY_KEY, AuthenticationField.PRIVILEGE_CATEGORY_VALUE_OPERATOR); } } - public ElasticsearchSecurityException check(String action, TransportRequest request, ThreadContext threadContext) { + public ElasticsearchSecurityException check( + Authentication authentication, String action, TransportRequest request, ThreadContext threadContext + ) { if (false == shouldProcess()) { return null; } + final User user = authentication.getUser(); + // Let internal users pass (also check run_as, it is impossible to run_as internal users, but just to be extra safe) + if (User.isInternal(user) && false == user.isRunAs()) { + return null; + } if (false == AuthenticationField.PRIVILEGE_CATEGORY_VALUE_OPERATOR.equals( threadContext.getHeader(AuthenticationField.PRIVILEGE_CATEGORY_KEY))) { // Only check whether request is operator-only when user is NOT an operator - logger.trace("Checking operator-only violation for: action [{}]", action); + logger.trace("Checking operator-only violation for user [{}] and action [{}]", user, action); final OperatorOnlyRegistry.OperatorPrivilegesViolation violation = operatorOnlyRegistry.check(action, request); if (violation != null) { return new ElasticsearchSecurityException("Operator privileges are required for " + violation.message()); @@ -87,6 +104,7 @@ public ElasticsearchSecurityException check(String action, TransportRequest requ public void maybeInterceptRequest(ThreadContext threadContext, TransportRequest request) { if (request instanceof RestoreSnapshotRequest) { + logger.debug("Intercepting [{}] for operator privileges", request); ((RestoreSnapshotRequest) request).skipOperatorOnlyState(shouldProcess()); } } @@ -102,7 +120,9 @@ public void maybeMarkOperatorUser(Authentication authentication, ThreadContext t } @Override - public ElasticsearchSecurityException check(String action, TransportRequest request, ThreadContext threadContext) { + public ElasticsearchSecurityException check( + Authentication authentication, String action, TransportRequest request, ThreadContext threadContext + ) { return null; } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java index cf2afb7a75e44..08f0e51c70fd4 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java @@ -365,7 +365,7 @@ private void authorize( final ElasticsearchSecurityException operatorPrivilegesException = new ElasticsearchSecurityException("Operator privileges check failed"); if (shouldFailOperatorPrivilegesCheck) { - when(operatorPrivilegesService.check(action, request, threadContext)).thenAnswer(invocationOnMock -> { + when(operatorPrivilegesService.check(authentication, action, request, threadContext)).thenAnswer(invocationOnMock -> { operatorPrivilegesChecked.set(true); return operatorPrivilegesException; }); @@ -377,7 +377,7 @@ private void authorize( authorizationInfo.onResponse(threadContext.getTransient(AUTHORIZATION_INFO_KEY)); indicesPermissions.onResponse(threadContext.getTransient(INDICES_PERMISSIONS_KEY)); - assertNull(verify(operatorPrivilegesService).check(action, request, threadContext)); + assertNull(verify(operatorPrivilegesService).check(authentication, action, request, threadContext)); if (listenerBody != null) { listenerBody.run(); diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStoreTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStoreTests.java index ccd0f2ce31f7c..65e971ed31e00 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStoreTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStoreTests.java @@ -26,11 +26,7 @@ import org.elasticsearch.watcher.ResourceWatcherService; import org.elasticsearch.xpack.core.security.audit.logfile.CapturingLogger; import org.elasticsearch.xpack.core.security.authc.Authentication; -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.junit.After; import org.junit.Before; @@ -98,18 +94,6 @@ public void testIsOperator() throws IOException { assertFalse(fileOperatorUsersStore.isOperatorUser( new Authentication(operator_1, fileRealm, fileRealm, Version.CURRENT, Authentication.AuthenticationType.TOKEN, Map.of()))); - - // Run as user operator_1 is not an operator - final User runAsOperator_1 = new User(operator_1, new User(randomAlphaOfLengthBetween(5, 8), randomRoles())); - assertFalse(fileOperatorUsersStore.isOperatorUser(new Authentication(runAsOperator_1, fileRealm, fileRealm))); - - // Internal users are operator - final Authentication.RealmRef realm = - new Authentication.RealmRef(randomAlphaOfLength(8), randomAlphaOfLength(8), randomAlphaOfLength(8)); - final Authentication authentication = new Authentication( - randomFrom(SystemUser.INSTANCE, XPackUser.INSTANCE, XPackSecurityUser.INSTANCE, AsyncSearchUser.INSTANCE), - realm, realm); - assertTrue(fileOperatorUsersStore.isOperatorUser(authentication)); } public void testFileAutoReload() throws Exception { @@ -120,19 +104,19 @@ public void testFileAutoReload() throws Exception { final Logger logger = LogManager.getLogger(FileOperatorUsersStore.class); final MockLogAppender appender = new MockLogAppender(); appender.start(); - appender.addExpectation( - new MockLogAppender.ExceptionSeenEventExpectation( - getTestName(), - logger.getName(), - Level.ERROR, - "Failed to parse operator users file", - XContentParseException.class, - "[10:1] [operator_privileges.operator] failed to parse field [operator]" - ) - ); Loggers.addAppender(logger, appender); + Loggers.setLevel(logger, Level.TRACE); try (ResourceWatcherService watcherService = new ResourceWatcherService(settings, threadPool)) { + appender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "1st file parsing", + logger.getName(), + Level.INFO, + "parsed [2] group(s) with a total of [3] operator user(s) from file [" + inUseFile.toAbsolutePath() + "]" + ) + ); + final FileOperatorUsersStore fileOperatorUsersStore = new FileOperatorUsersStore(env, watcherService); final List groups = fileOperatorUsersStore.getOperatorUsersDescriptor().getGroups(); @@ -142,6 +126,7 @@ public void testFileAutoReload() throws Exception { "file"), groups.get(0)); assertEquals(new FileOperatorUsersStore.Group( Set.of("operator_3"), null), groups.get(1)); + appender.assertAllExpectationsMatched(); // Content does not change, the groups should not be updated try (BufferedWriter writer = Files.newBufferedWriter(inUseFile, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { @@ -149,8 +134,17 @@ public void testFileAutoReload() throws Exception { } watcherService.notifyNow(ResourceWatcherService.Frequency.HIGH); assertSame(groups, fileOperatorUsersStore.getOperatorUsersDescriptor().getGroups()); + appender.assertAllExpectationsMatched(); // Add one more entry + appender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "updating", + logger.getName(), + Level.INFO, + "operator users file [" + inUseFile.toAbsolutePath() + "] changed. updating operator users" + ) + ); try (BufferedWriter writer = Files.newBufferedWriter(inUseFile, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { writer.append(" - usernames: [ 'operator_4' ]\n"); } @@ -160,8 +154,19 @@ public void testFileAutoReload() throws Exception { assertEquals(new FileOperatorUsersStore.Group( Set.of("operator_4")), newGroups.get(2)); }); + appender.assertAllExpectationsMatched(); // Add mal-formatted entry + appender.addExpectation( + new MockLogAppender.ExceptionSeenEventExpectation( + "mal-formatted", + logger.getName(), + Level.ERROR, + "Failed to parse operator users file", + XContentParseException.class, + "[10:1] [operator_privileges.operator] failed to parse field [operator]" + ) + ); try (BufferedWriter writer = Files.newBufferedWriter(inUseFile, StandardCharsets.UTF_8, StandardOpenOption.APPEND)) { writer.append(" - blah\n"); } @@ -170,8 +175,18 @@ public void testFileAutoReload() throws Exception { appender.assertAllExpectationsMatched(); // Delete the file will remove all the operator users + appender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "file not exist warning", + logger.getName(), + Level.WARN, + "Operator privileges [xpack.security.operator_privileges.enabled] is enabled, " + + "but operator user file does not exist. No user will be able to perform operator-only actions." + ) + ); Files.delete(inUseFile); assertBusy(() -> assertEquals(0, fileOperatorUsersStore.getOperatorUsersDescriptor().getGroups().size())); + appender.assertAllExpectationsMatched(); // Back to original content Files.copy(sampleFile, inUseFile, StandardCopyOption.REPLACE_EXISTING); @@ -179,6 +194,7 @@ public void testFileAutoReload() throws Exception { } finally { Loggers.removeAppender(logger, appender); appender.stop(); + Loggers.setLevel(logger, (Level) null); } } diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesTests.java index 45512fce7796e..32eafc7a52a83 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesTests.java @@ -7,25 +7,37 @@ package org.elasticsearch.xpack.security.operator; +import org.apache.logging.log4j.Level; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.elasticsearch.ElasticsearchSecurityException; import org.elasticsearch.action.admin.cluster.snapshots.restore.RestoreSnapshotRequest; +import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.ThreadContext; import org.elasticsearch.license.XPackLicenseState; import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.MockLogAppender; import org.elasticsearch.transport.TransportRequest; import org.elasticsearch.xpack.core.security.authc.Authentication; import org.elasticsearch.xpack.core.security.authc.AuthenticationField; +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.operator.OperatorPrivileges.DefaultOperatorPrivilegesService; import org.elasticsearch.xpack.security.operator.OperatorPrivileges.OperatorPrivilegesService; import org.junit.Before; +import org.mockito.Mockito; import static org.elasticsearch.xpack.security.operator.OperatorPrivileges.NOOP_OPERATOR_PRIVILEGES_SERVICE; import static org.hamcrest.Matchers.containsString; import static org.mockito.Matchers.any; +import static org.mockito.Matchers.anyString; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.verifyZeroInteractions; import static org.mockito.Mockito.when; @@ -56,30 +68,74 @@ public void testWillNotProcessWhenFeatureIsDisabledOrLicenseDoesNotSupport() { verifyZeroInteractions(fileOperatorUsersStore); final ElasticsearchSecurityException e = - operatorPrivilegesService.check("cluster:action", mock(TransportRequest.class), threadContext); + operatorPrivilegesService.check(mock(Authentication.class), "cluster:action", mock(TransportRequest.class), threadContext); assertNull(e); verifyZeroInteractions(operatorOnlyRegistry); } - public void testMarkOperatorUser() { + public void testMarkOperatorUser() throws IllegalAccessException { final Settings settings = Settings.builder() .put("xpack.security.operator_privileges.enabled", true) .build(); when(xPackLicenseState.checkFeature(XPackLicenseState.Feature.OPERATOR_PRIVILEGES)).thenReturn(true); final Authentication operatorAuth = mock(Authentication.class); final Authentication nonOperatorAuth = mock(Authentication.class); - when(operatorAuth.getUser()).thenReturn(new User("operator_user")); + final User operatorUser = new User("operator_user"); + when(operatorAuth.getUser()).thenReturn(operatorUser); + when(nonOperatorAuth.getUser()).thenReturn(new User("non_operator_user")); when(fileOperatorUsersStore.isOperatorUser(operatorAuth)).thenReturn(true); when(fileOperatorUsersStore.isOperatorUser(nonOperatorAuth)).thenReturn(false); ThreadContext threadContext = new ThreadContext(settings); - operatorPrivilegesService.maybeMarkOperatorUser(operatorAuth, threadContext); - assertEquals(AuthenticationField.PRIVILEGE_CATEGORY_VALUE_OPERATOR, - threadContext.getHeader(AuthenticationField.PRIVILEGE_CATEGORY_KEY)); + // Will mark for the operator user + final Logger logger = LogManager.getLogger(OperatorPrivileges.class); + final MockLogAppender appender = new MockLogAppender(); + appender.start(); + Loggers.addAppender(logger, appender); + Loggers.setLevel(logger, Level.DEBUG); + + try { + appender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "marking", + logger.getName(), + Level.DEBUG, + "Marking user [" + operatorUser + "] as an operator" + ) + ); + operatorPrivilegesService.maybeMarkOperatorUser(operatorAuth, threadContext); + assertEquals(AuthenticationField.PRIVILEGE_CATEGORY_VALUE_OPERATOR, + threadContext.getHeader(AuthenticationField.PRIVILEGE_CATEGORY_KEY)); + appender.assertAllExpectationsMatched(); + } finally { + Loggers.removeAppender(logger, appender); + appender.stop(); + Loggers.setLevel(logger, (Level) null); + } + // Will not mark for non-operator user threadContext = new ThreadContext(settings); operatorPrivilegesService.maybeMarkOperatorUser(nonOperatorAuth, threadContext); assertNull(threadContext.getHeader(AuthenticationField.PRIVILEGE_CATEGORY_KEY)); + + // Will not mark for run_as user + final Authentication runAsAuth = mock(Authentication.class); + when(runAsAuth.getUser()).thenReturn(new User(operatorUser, operatorUser)); + Mockito.reset(fileOperatorUsersStore); + when(fileOperatorUsersStore.isOperatorUser(runAsAuth)).thenReturn(true); + threadContext = new ThreadContext(settings); + operatorPrivilegesService.maybeMarkOperatorUser(runAsAuth, threadContext); + assertNull(threadContext.getHeader(AuthenticationField.PRIVILEGE_CATEGORY_KEY)); + verify(fileOperatorUsersStore, never()).isOperatorUser(any()); + + // Will not mark for internal users + final Authentication internalAuth = mock(Authentication.class); + when(internalAuth.getUser()).thenReturn( + randomFrom(SystemUser.INSTANCE, XPackUser.INSTANCE, XPackSecurityUser.INSTANCE, AsyncSearchUser.INSTANCE)); + threadContext = new ThreadContext(settings); + operatorPrivilegesService.maybeMarkOperatorUser(runAsAuth, threadContext); + assertNull(threadContext.getHeader(AuthenticationField.PRIVILEGE_CATEGORY_KEY)); + verify(fileOperatorUsersStore, never()).isOperatorUser(any()); } public void testCheck() { @@ -94,28 +150,60 @@ public void testCheck() { when(operatorOnlyRegistry.check(eq(operatorAction), any())).thenReturn(() -> message); when(operatorOnlyRegistry.check(eq(nonOperatorAction), any())).thenReturn(null); ThreadContext threadContext = new ThreadContext(settings); + final Authentication authentication = mock(Authentication.class); + when(authentication.getUser()).thenReturn(new User(randomAlphaOfLengthBetween(3, 8))); if (randomBoolean()) { threadContext.putHeader(AuthenticationField.PRIVILEGE_CATEGORY_KEY, AuthenticationField.PRIVILEGE_CATEGORY_VALUE_OPERATOR); - assertNull(operatorPrivilegesService.check(operatorAction, mock(TransportRequest.class), threadContext)); + assertNull(operatorPrivilegesService.check(authentication, operatorAction, mock(TransportRequest.class), threadContext)); } else { final ElasticsearchSecurityException e = operatorPrivilegesService.check( - operatorAction, mock(TransportRequest.class), threadContext); + authentication, operatorAction, mock(TransportRequest.class), threadContext); assertNotNull(e); assertThat(e.getMessage(), containsString("Operator privileges are required for " + message)); } - assertNull(operatorPrivilegesService.check(nonOperatorAction, mock(TransportRequest.class), threadContext)); + assertNull(operatorPrivilegesService.check(authentication, nonOperatorAction, mock(TransportRequest.class), threadContext)); + } + + public void testCheckWillPassForInternalUsers() { + when(xPackLicenseState.checkFeature(XPackLicenseState.Feature.OPERATOR_PRIVILEGES)).thenReturn(true); + final Authentication internalAuth = mock(Authentication.class); + when(internalAuth.getUser()).thenReturn( + randomFrom(SystemUser.INSTANCE, XPackUser.INSTANCE, XPackSecurityUser.INSTANCE, AsyncSearchUser.INSTANCE)); + assertNull(operatorPrivilegesService.check( + internalAuth, randomAlphaOfLengthBetween(20, 30), mock(TransportRequest.class), new ThreadContext(Settings.EMPTY))); + verify(operatorOnlyRegistry, never()).check(anyString(), any()); } - public void testMaybeInterceptRequest() { + public void testMaybeInterceptRequest() throws IllegalAccessException { final boolean licensed = randomBoolean(); when(xPackLicenseState.checkFeature(XPackLicenseState.Feature.OPERATOR_PRIVILEGES)).thenReturn(licensed); - final RestoreSnapshotRequest restoreSnapshotRequest = mock(RestoreSnapshotRequest.class); - operatorPrivilegesService.maybeInterceptRequest(new ThreadContext(Settings.EMPTY), restoreSnapshotRequest); - - verify(restoreSnapshotRequest).skipOperatorOnlyState(licensed); + final Logger logger = LogManager.getLogger(OperatorPrivileges.class); + final MockLogAppender appender = new MockLogAppender(); + appender.start(); + Loggers.addAppender(logger, appender); + Loggers.setLevel(logger, Level.DEBUG); + + try { + final RestoreSnapshotRequest restoreSnapshotRequest = mock(RestoreSnapshotRequest.class); + appender.addExpectation( + new MockLogAppender.SeenEventExpectation( + "intercepting", + logger.getName(), + Level.DEBUG, + "Intercepting [" + restoreSnapshotRequest + "] for operator privileges" + ) + ); + operatorPrivilegesService.maybeInterceptRequest(new ThreadContext(Settings.EMPTY), restoreSnapshotRequest); + verify(restoreSnapshotRequest).skipOperatorOnlyState(licensed); + appender.assertAllExpectationsMatched(); + } finally { + Loggers.removeAppender(logger, appender); + appender.stop(); + Loggers.setLevel(logger, (Level) null); + } } public void testMaybeInterceptRequestWillNotInterceptRequestsOtherThanRestoreSnapshotRequest() { @@ -133,7 +221,7 @@ public void testNoOpService() { final TransportRequest request = mock(TransportRequest.class); assertNull(NOOP_OPERATOR_PRIVILEGES_SERVICE.check( - randomAlphaOfLengthBetween(10, 20), request, threadContext)); + mock(Authentication.class), randomAlphaOfLengthBetween(10, 20), request, threadContext)); verifyZeroInteractions(request); }