Skip to content

Commit

Permalink
Better logging and internal user handling for operator privileges (#7…
Browse files Browse the repository at this point in the history 
…9331) (#79336)

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.
  • Loading branch information
@ywangd
ywangd authored Oct 18, 2021
1 parent f37d528 commit 851a8ce
Show file tree
Hide file tree
Showing 7 changed files with 193 additions and 66 deletions.
1 change: 1 addition & 0 deletions ...src/javaRestTest/java/org/elasticsearch/xpack/security/operator/OperatorPrivilegesIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
6 changes: 4 additions & 2 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 @@ -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);
}
Expand Down
28 changes: 14 additions & 14 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStore.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -65,24 +64,20 @@ 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.
// The special handling for realm name is because there can only be one file or native realm and it does
// 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;
});
}

Expand Down Expand Up @@ -218,19 +213,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.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());
}
}
}

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);
}
}

Expand Down
34 changes: 27 additions & 7 deletions .../security/src/main/java/org/elasticsearch/xpack/security/operator/OperatorPrivileges.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {

Expand All @@ -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);
}
Expand All @@ -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());
Expand All @@ -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());
}
}
Expand All @@ -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;
}

Expand Down
4 changes: 2 additions & 2 deletions ...urity/src/test/java/org/elasticsearch/xpack/security/authz/AuthorizationServiceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
});
Expand All @@ -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();
Expand Down
68 changes: 42 additions & 26 deletions .../src/test/java/org/elasticsearch/xpack/security/operator/FileOperatorUsersStoreTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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 {
Expand All @@ -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<FileOperatorUsersStore.Group> groups = fileOperatorUsersStore.getOperatorUsersDescriptor().getGroups();

Expand All @@ -142,15 +126,25 @@ 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)) {
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");
}
Expand All @@ -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");
}
Expand All @@ -170,15 +175,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);
}
}

Expand Down
Loading

0 comments on commit 851a8ce

Please sign in to comment.