Better logging and internal user handling for operator privileges

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java

@@ -285,9 +285,10 @@ 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, "and operator privileges", operatorException);
         }
         operatorPrivilegesService.maybeInterceptRequest(threadContext, originalRequest);
     }

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.
@@ -217,19 +209,24 @@ 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.trace("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());
             }
         }
     }
 
-    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);
         }
     }
 

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,35 @@ public void maybeMarkOperatorUser(Authentication authentication, ThreadContext t
             if (false == shouldProcess()) {
                 return;
             }
+            // Let internal users pass, they are exempt from marking and checking
+            if (User.isInternal(authentication.getUser())) {
+                return;
+            }
+            // Do not support run_as operator user, checking will fail later if this action requires operator privilege
+            if (authentication.getUser().isRunAs()) {
+                return;
+            }
             if (fileOperatorUsersStore.isOperatorUser(authentication)) {
-                logger.trace("User [{}] is an operator", authentication.getUser().principal());
+                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 +107,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 +123,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;
         }
 

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java

@@ -364,7 +364,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;
             });
@@ -376,7 +376,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();

x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStoreTests.java

@@ -24,11 +24,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;
 
@@ -97,18 +93,6 @@ public void testIsOperator() throws IOException {
         // user operator_1 with non realm auth type is not an operator
         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 {
@@ -119,35 +103,45 @@ 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<FileOperatorUsersStore.Group> groups = fileOperatorUsersStore.getOperatorUsersDescriptor().getGroups();
 
             assertEquals(2, groups.size());
             assertEquals(new FileOperatorUsersStore.Group(Set.of("operator_1", "operator_2"),
                 "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)) {
                 writer.append("\n");
             }
             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");
             }
@@ -156,8 +150,19 @@ public void testFileAutoReload() throws Exception {
                 assertEquals(3, newGroups.size());
                 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");
             }
@@ -166,15 +171,26 @@ 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);
             assertBusy(() -> assertEquals(2, fileOperatorUsersStore.getOperatorUsersDescriptor().getGroups().size()));
         } finally {
             Loggers.removeAppender(logger, appender);
             appender.stop();
+            Loggers.setLevel(logger, (Level) null);
         }
     }