Skip to content

Commit

Permalink
Add info of effective roles in denial messages (#89680)
Browse files Browse the repository at this point in the history
When an action is denied due to authorization error, the list of
assigned roles is shown in the error message. However, it is possible
that the effective roles are fewer or more than the assigned list: *
Fewer roles can happen when the role is not defined or the license does
not permit it * More roles can happen when anonymous access is enabled

This PR changes the error message to show the effective roles instead of
the assigned roles (whenever possible) to help troubleshooting. In
addition, it also reports any missing roles, i.e. roles that are
assigned but cannot be found.
  • Loading branch information
ywangd committed Sep 1, 2022
1 parent 69a31da commit adf8e01
Show file tree
Hide file tree
Showing 14 changed files with 341 additions and 56 deletions.
5 changes: 5 additions & 0 deletions docs/changelog/89680.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 89680
summary: Add info of resolved roles in denial messages
area: Authorization
type: enhancement
issues: []
12 changes: 12 additions & 0 deletions x-pack/plugin/build.gradle
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,18 @@ tasks.named("yamlRestTestV7CompatTransform").configure { task ->
'/It is no longer possible to freeze indices, but existing frozen indices can still be unfrozen/',
"Cannot freeze write index for data stream"
)

task.replaceValueInMatch(
"error.reason",
"action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [api_key_user_1] with effective roles [user_role], this action is granted by the cluster privileges [manage_api_key,manage_security,all]",
"Test invalidate api key by realm name"
)

task.replaceValueInMatch(
"error.reason",
"action [cluster:admin/xpack/security/api_key/invalidate] is unauthorized for user [api_key_user_1] with effective roles [user_role], this action is granted by the cluster privileges [manage_api_key,manage_security,all]",
"Test invalidate api key by username"
)
}


Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ public void testCanManageIndexWithNoPermissions() throws Exception {
equalTo(
"action [indices:monitor/stats] is unauthorized"
+ " for user [test_ilm]"
+ " with roles [ilm]"
+ " with effective roles [ilm]"
+ " on indices [not-ilm],"
+ " this action is granted by the index privileges [monitor,manage,all]"
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1086,7 +1086,7 @@ public void testLookbackWithoutPermissions() throws Exception {
"\"message\":\"Datafeed is encountering errors extracting data: "
+ "action [indices:data/read/search] is unauthorized"
+ " for user [ml_admin_plus_data]"
+ " with roles [machine_learning_admin,test_data_access]"
+ " with effective roles [machine_learning_admin,test_data_access]"
+ " on indices [network-data]"
)
);
Expand Down Expand Up @@ -1286,7 +1286,7 @@ public void testLookbackWithoutPermissionsAndRollup() throws Exception {
"\"message\":\"Datafeed is encountering errors extracting data: "
+ "action [indices:data/read/xpack/rollup/search] is unauthorized"
+ " for user [ml_admin_plus_data]"
+ " with roles [machine_learning_admin,test_data_access]"
+ " with effective roles [machine_learning_admin,test_data_access]"
+ " on indices [airline-data-aggs-rollup]"
)
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,9 @@ public void onResponse(ResetFeatureStateResponse response) {
public void onFailure(Exception e) {
assertThat(
e.getMessage(),
containsString("action [cluster:admin/features/reset] is unauthorized for user [usr] with roles [role2]")
containsString(
"action [cluster:admin/features/reset] is unauthorized for user [usr]" + " with effective roles [role2]"
)
);
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -308,7 +308,8 @@ public void testGrantApiKeyForUserWithRunAs() throws IOException {
e1.getMessage(),
containsString(
"action [cluster:admin/xpack/security/user/authenticate] is unauthorized "
+ "for user [user1] because user [user1] is unauthorized to run as [user3]"
+ "for user [user1] with effective roles [user1_role]"
+ ", because user [user1] is unauthorized to run as [user3]"
)
);

Expand All @@ -322,7 +323,8 @@ public void testGrantApiKeyForUserWithRunAs() throws IOException {
e2.getMessage(),
containsString(
"action [cluster:admin/xpack/security/user/authenticate] is unauthorized "
+ "for user [user1] because user [user1] is unauthorized to run as [user4]"
+ "for user [user1] with effective roles [user1_role]"
+ ", because user [user1] is unauthorized to run as [user4]"
)
);

Expand Down Expand Up @@ -368,7 +370,8 @@ public void testGrantApiKeyForUserWithRunAs() throws IOException {
e4.getMessage(),
containsString(
"action [cluster:admin/xpack/security/user/authenticate] is unauthorized "
+ "for user [user1] because user [user1] is unauthorized to run as [user4]"
+ "for user [user1] with effective roles [user1_role]"
+ ", because user [user1] is unauthorized to run as [user4]"
)
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,23 +8,33 @@
package org.elasticsearch.xpack.security.authz;

import org.elasticsearch.common.Strings;
import org.elasticsearch.common.util.set.Sets;
import org.elasticsearch.core.Nullable;
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.authc.Subject;
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.AuthorizationInfo;
import org.elasticsearch.xpack.core.security.authz.permission.Role;
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;

import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.SortedSet;

import static org.elasticsearch.common.Strings.collectionToCommaDelimitedString;
import static org.elasticsearch.xpack.security.audit.logfile.LoggingAuditTrail.PRINCIPAL_ROLES_FIELD_NAME;
import static org.elasticsearch.xpack.security.authz.AuthorizationService.isIndexAction;

class AuthorizationDenialMessages {

private AuthorizationDenialMessages() {}

static String runAsDenied(Authentication authentication, String action) {
static String runAsDenied(Authentication authentication, @Nullable AuthorizationInfo authorizationInfo, String action) {
assert authentication.isRunAs() : "constructing run as denied message but authentication for action was not run as";

String userText = authenticatedUserDescription(authentication);
Expand All @@ -36,22 +46,26 @@ static String runAsDenied(Authentication authentication, String action) {
+ authentication.getUser().principal()
+ "]";

return actionIsUnauthorizedMessage + " " + unauthorizedToRunAsMessage;
return actionIsUnauthorizedMessage
+ rolesDescription(authentication.getAuthenticatingSubject(), authorizationInfo.getAuthenticatedUserAuthorizationInfo())
+ ", "
+ unauthorizedToRunAsMessage;
}

static String actionDenied(Authentication authentication, String action, TransportRequest request, @Nullable String context) {
static String actionDenied(
Authentication authentication,
@Nullable AuthorizationInfo authorizationInfo,
String action,
TransportRequest request,
@Nullable String context
) {
String userText = authenticatedUserDescription(authentication);

if (authentication.isRunAs()) {
userText = userText + " run as [" + authentication.getUser().principal() + "]";
}

// The run-as user is always from a realm. So it must have roles that can be printed.
// If the user is not run-as, we cannot print the roles if it's an API key or a service account (both do not have
// roles, but privileges)
if (false == authentication.isServiceAccount() && false == authentication.isApiKey()) {
userText = userText + " with roles [" + Strings.arrayToCommaDelimitedString(authentication.getUser().roles()) + "]";
}
userText += rolesDescription(authentication.getEffectiveSubject(), authorizationInfo);

String message = actionIsUnauthorizedMessage(action, userText);
if (context != null) {
Expand Down Expand Up @@ -92,6 +106,46 @@ private static String authenticatedUserDescription(Authentication authentication
return userText;
}

static String rolesDescription(Subject subject, @Nullable AuthorizationInfo authorizationInfo) {
// We cannot print the roles if it's an API key or a service account (both do not have roles, but privileges)
if (subject.getType() != Subject.Type.USER) {
return "";
}

final StringBuilder sb = new StringBuilder();
final List<String> effectiveRoleNames = extractEffectiveRoleNames(authorizationInfo);
if (effectiveRoleNames == null) {
sb.append(" with assigned roles [").append(Strings.arrayToCommaDelimitedString(subject.getUser().roles())).append("]");
} else {
sb.append(" with effective roles [").append(Strings.collectionToCommaDelimitedString(effectiveRoleNames)).append("]");

final Set<String> assignedRoleNames = Set.of(subject.getUser().roles());
final SortedSet<String> unfoundedRoleNames = Sets.sortedDifference(assignedRoleNames, Set.copyOf(effectiveRoleNames));
if (false == unfoundedRoleNames.isEmpty()) {
sb.append(" (assigned roles [")
.append(Strings.collectionToCommaDelimitedString(unfoundedRoleNames))
.append("] were not found)");
}
}
return sb.toString();
}

private static List<String> extractEffectiveRoleNames(@Nullable AuthorizationInfo authorizationInfo) {
if (authorizationInfo == null) {
return null;
}
final Role role = RBACEngine.maybeGetRBACEngineRole(authorizationInfo);
if (role == Role.EMPTY) {
return List.of();
} else {
final Map<String, Object> info = authorizationInfo.asMap();
if (false == info.containsKey(PRINCIPAL_ROLES_FIELD_NAME)) {
return null;
}
return Arrays.stream((String[]) info.get(PRINCIPAL_ROLES_FIELD_NAME)).sorted().toList();
}
}

private static String actionIsUnauthorizedMessage(String action, String userText) {
return "action [" + action + "] is unauthorized for " + userText;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -328,7 +328,7 @@ private void checkOperatorPrivileges(Authentication authentication, String actio
threadContext
);
if (operatorException != null) {
throw actionDenied(authentication, action, originalRequest, "because it requires operator privileges", operatorException);
throw actionDenied(authentication, null, action, originalRequest, "because it requires operator privileges", operatorException);
}
operatorPrivilegesService.maybeInterceptRequest(threadContext, originalRequest);
}
Expand Down Expand Up @@ -367,11 +367,11 @@ private void maybeAuthorizeRunAs(
authzInfo.getAuthenticatedUserAuthorizationInfo()
);
}
listener.onFailure(runAsDenied(authentication, action));
listener.onFailure(runAsDenied(authentication, authzInfo, action));
}
}, e -> {
auditTrail.runAsDenied(requestId, authentication, action, request, authzInfo.getAuthenticatedUserAuthorizationInfo());
listener.onFailure(actionDenied(authentication, action, request));
listener.onFailure(actionDenied(authentication, authzInfo, action, request));
}), threadContext);
authorizeRunAs(requestInfo, authzInfo, runAsListener);
} else {
Expand Down Expand Up @@ -431,7 +431,7 @@ private void authorizeAction(
if (e instanceof IndexNotFoundException) {
listener.onFailure(e);
} else {
listener.onFailure(actionDenied(authentication, action, request, e));
listener.onFailure(actionDenied(authentication, authzInfo, action, request, e));
}
}
)
Expand Down Expand Up @@ -466,7 +466,7 @@ private void authorizeAction(
} else {
logger.warn("denying access as action [{}] is not an index or cluster action", action);
auditTrail.accessDenied(requestId, authentication, action, request, authzInfo);
listener.onFailure(actionDenied(authentication, action, request));
listener.onFailure(actionDenied(authentication, authzInfo, action, request));
}
}

Expand Down Expand Up @@ -620,7 +620,7 @@ private void authorizeSystemUser(
listener.onResponse(null);
} else {
auditTrail.accessDenied(requestId, authentication, action, request, SYSTEM_AUTHZ_INFO);
listener.onFailure(actionDenied(authentication, action, request));
listener.onFailure(actionDenied(authentication, SYSTEM_AUTHZ_INFO, action, request));
}
}

Expand All @@ -644,14 +644,14 @@ private TransportRequest maybeUnwrapRequest(
"originalRequest is not a proxy request: [" + originalRequest + "] but action: [" + action + "] is a proxy action"
);
auditTrail.accessDenied(requestId, authentication, action, request, EmptyAuthorizationInfo.INSTANCE);
throw actionDenied(authentication, action, request, cause);
throw actionDenied(authentication, null, action, request, cause);
}
if (TransportActionProxy.isProxyRequest(originalRequest) && TransportActionProxy.isProxyAction(action) == false) {
IllegalStateException cause = new IllegalStateException(
"originalRequest is a proxy request for: [" + request + "] but action: [" + action + "] isn't"
);
auditTrail.accessDenied(requestId, authentication, action, request, EmptyAuthorizationInfo.INSTANCE);
throw actionDenied(authentication, action, request, cause);
throw actionDenied(authentication, null, action, request, cause);
}
}
return request;
Expand Down Expand Up @@ -680,7 +680,7 @@ private void authorizeRunAs(
* and then checks whether that action is allowed on the targeted index. Items
* that fail this checks are {@link BulkItemRequest#abort(String, Exception)
* aborted}, with an
* {@link #actionDenied(Authentication, String, TransportRequest, String, Exception)
* {@link #actionDenied(Authentication, AuthorizationInfo, String, TransportRequest, String, Exception)
* access denied} exception. Because a shard level request is for exactly 1 index,
* and there are a small number of possible item {@link DocWriteRequest.OpType
* types}, the number of distinct authorization checks that need to be performed
Expand Down Expand Up @@ -781,6 +781,7 @@ private void authorizeBulkItems(
resolvedIndex,
actionDenied(
authentication,
authzInfo,
itemAction,
request,
IndexAuthorizationResult.getFailureDescription(List.of(resolvedIndex), restrictedIndices),
Expand Down Expand Up @@ -856,25 +857,41 @@ private void putTransientIfNonExisting(String key, Object value) {
}
}

private ElasticsearchSecurityException runAsDenied(Authentication authentication, String action) {
return denialException(authentication, action, () -> AuthorizationDenialMessages.runAsDenied(authentication, action), null);
private ElasticsearchSecurityException runAsDenied(
Authentication authentication,
@Nullable AuthorizationInfo authorizationInfo,
String action
) {
return denialException(
authentication,
action,
() -> AuthorizationDenialMessages.runAsDenied(authentication, authorizationInfo, action),
null
);
}

private ElasticsearchSecurityException actionDenied(Authentication authentication, String action, TransportRequest request) {
return actionDenied(authentication, action, request, null);
private ElasticsearchSecurityException actionDenied(
Authentication authentication,
@Nullable AuthorizationInfo authorizationInfo,
String action,
TransportRequest request
) {
return actionDenied(authentication, authorizationInfo, action, request, null);
}

private ElasticsearchSecurityException actionDenied(
Authentication authentication,
@Nullable AuthorizationInfo authorizationInfo,
String action,
TransportRequest request,
Exception cause
) {
return actionDenied(authentication, action, request, null, cause);
return actionDenied(authentication, authorizationInfo, action, request, null, cause);
}

private ElasticsearchSecurityException actionDenied(
Authentication authentication,
@Nullable AuthorizationInfo authorizationInfo,
String action,
TransportRequest request,
@Nullable String context,
Expand All @@ -883,7 +900,7 @@ private ElasticsearchSecurityException actionDenied(
return denialException(
authentication,
action,
() -> AuthorizationDenialMessages.actionDenied(authentication, action, request, context),
() -> AuthorizationDenialMessages.actionDenied(authentication, authorizationInfo, action, request, context),
cause
);
}
Expand Down Expand Up @@ -963,7 +980,7 @@ private void handleFailure(boolean audit, @Nullable String context, @Nullable Ex
if (audit) {
auditTrailService.get().accessDenied(requestId, authentication, action, request, authzInfo);
}
failureConsumer.accept(actionDenied(authentication, action, request, context, e));
failureConsumer.accept(actionDenied(authentication, authzInfo, action, request, context, e));
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import org.elasticsearch.common.Strings;
import org.elasticsearch.license.LicenseUtils;
import org.elasticsearch.rest.RestStatus;
import org.elasticsearch.xpack.core.security.user.User;
import org.hamcrest.Matcher;

import static org.apache.lucene.tests.util.LuceneTestCase.expectThrows;
Expand Down Expand Up @@ -62,19 +63,27 @@ public static void assertThrowsAuthorizationException(
public static void assertThrowsAuthorizationExceptionRunAsDenied(
LuceneTestCase.ThrowingRunnable throwingRunnable,
String action,
String user,
User authenticatingUser,
String runAs
) {
assertThrowsAuthorizationException(
"Expected authorization failure for user=[" + user + "], run-as=[" + runAs + "], action=[" + action + "]",
"Expected authorization failure for user=["
+ authenticatingUser.principal()
+ "], run-as=["
+ runAs
+ "], action=["
+ action
+ "]",
throwingRunnable,
containsString(
"action ["
+ action
+ "] is unauthorized for user ["
+ user
+ "] because user ["
+ user
+ authenticatingUser.principal()
+ "]"
+ " with effective roles [%s]".formatted(Strings.arrayToCommaDelimitedString(authenticatingUser.roles()))
+ ", because user ["
+ authenticatingUser.principal()
+ "] is unauthorized to run as ["
+ runAs
+ "]"
Expand Down

0 comments on commit adf8e01

Please sign in to comment.