Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return 400 error for GetUserPrivileges call with API keys #89333

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions docs/changelog/89333.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
pr: 89333
summary: Return 400 error for `GetUserPrivileges` call with API keys
area: Security
type: enhancement
issues: []
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@
import org.elasticsearch.client.Response;
import org.elasticsearch.client.ResponseException;
import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.core.Tuple;
import org.elasticsearch.test.XContentTestUtils;
import org.elasticsearch.xcontent.XContentParser;
import org.elasticsearch.xcontent.XContentType;
import org.elasticsearch.xcontent.json.JsonXContent;
import org.elasticsearch.xpack.core.security.action.apikey.ApiKey;
import org.elasticsearch.xpack.core.security.action.apikey.GetApiKeyResponse;
import org.elasticsearch.xpack.core.security.action.apikey.GrantApiKeyAction;
Expand Down Expand Up @@ -490,6 +492,87 @@ public void testGrantorCannotUpdateApiKeyOfGrantTarget() throws IOException {
);
}

public void testGetPrivilegesForApiKeyWorksIfItDoesNotHaveAssignedPrivileges() throws IOException {
final Request createApiKeyRequest = new Request("POST", "_security/api_key");
if (randomBoolean()) {
createApiKeyRequest.setJsonEntity("""
{ "name": "k1" }""");
} else {
createApiKeyRequest.setJsonEntity("""
{
"name": "k1",
"role_descriptors": { }
}""");
}
final Response createApiKeyResponse = adminClient().performRequest(createApiKeyRequest);
assertOK(createApiKeyResponse);

final Request getPrivilegesRequest = new Request("GET", "_security/user/_privileges");
getPrivilegesRequest.setOptions(
RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", "ApiKey " + responseAsMap(createApiKeyResponse).get("encoded"))
);
final Response getPrivilegesResponse = client().performRequest(getPrivilegesRequest);
assertOK(getPrivilegesResponse);

assertThat(responseAsMap(getPrivilegesResponse), equalTo(XContentHelper.convertToMap(JsonXContent.jsonXContent, """
{
"cluster": [
"all"
],
"global": [],
"indices": [
{
"names": [
"*"
],
"privileges": [
"all"
],
"allow_restricted_indices": true
}
],
"applications": [
{
"application": "*",
"privileges": [
"*"
],
"resources": [
"*"
]
}
],
"run_as": [
"*"
]
}""", false)));
}

public void testGetPrivilegesForApiKeyThrows400IfItHasAssignedPrivileges() throws IOException {
final Request createApiKeyRequest = new Request("POST", "_security/api_key");
createApiKeyRequest.setJsonEntity("""
{
"name": "k1",
"role_descriptors": { "a": { "cluster": ["monitor"] } }
}""");
final Response createApiKeyResponse = adminClient().performRequest(createApiKeyRequest);
assertOK(createApiKeyResponse);

final Request getPrivilegesRequest = new Request("GET", "_security/user/_privileges");
getPrivilegesRequest.setOptions(
RequestOptions.DEFAULT.toBuilder().addHeader("Authorization", "ApiKey " + responseAsMap(createApiKeyResponse).get("encoded"))
);
final ResponseException e = expectThrows(ResponseException.class, () -> client().performRequest(getPrivilegesRequest));
assertThat(e.getResponse().getStatusLine().getStatusCode(), equalTo(400));
assertThat(
e.getMessage(),
containsString(
"Cannot retrieve privileges for API keys with assigned role descriptors. "
+ "Please use the Get API key information API https://ela.st/es-api-get-api-key"
)
);
}

private void doTestAuthenticationWithApiKey(final String apiKeyName, final String apiKeyId, final String apiKeyEncoded)
throws IOException {
final var authenticateRequest = new Request("GET", "_security/_authenticate");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,17 @@ public void getUserPrivileges(AuthorizationInfo authorizationInfo, ActionListene
);
} else {
final Role role = ((RBACAuthorizationInfo) authorizationInfo).getRole();
listener.onResponse(buildUserPrivilegesResponseObject(role));
final GetUserPrivilegesResponse getUserPrivilegesResponse;
try {
getUserPrivilegesResponse = buildUserPrivilegesResponseObject(role);
} catch (UnsupportedOperationException e) {
throw new IllegalArgumentException(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: to stay consistent with the other exception above, should we pass this to onFailure instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was trying to be consistent with the existing behaviour, i.e. buildUserPrivilegesResponseObject just throws exceptions. But consistent with the caller method is also good. I'll make the change

"Cannot retrieve privileges for API keys with assigned role descriptors. "
+ "Please use the Get API key information API https://ela.st/es-api-get-api-key",
e
);
}
listener.onResponse(getUserPrivilegesResponse);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,14 @@
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.PrivilegesToCheck;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.ApplicationResourcePrivileges;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor.IndicesPrivileges;
import org.elasticsearch.xpack.core.security.authz.permission.ApplicationPermission;
import org.elasticsearch.xpack.core.security.authz.permission.ClusterPermission;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissions;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissionsDefinition;
import org.elasticsearch.xpack.core.security.authz.permission.IndicesPermission;
import org.elasticsearch.xpack.core.security.authz.permission.ResourcePrivileges;
import org.elasticsearch.xpack.core.security.authz.permission.Role;
import org.elasticsearch.xpack.core.security.authz.permission.RunAsPermission;
import org.elasticsearch.xpack.core.security.authz.permission.SimpleRole;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilege;
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
Expand Down Expand Up @@ -105,6 +109,7 @@
import static org.hamcrest.Matchers.iterableWithSize;
import static org.hamcrest.Matchers.notNullValue;
import static org.hamcrest.Matchers.nullValue;
import static org.hamcrest.Matchers.sameInstance;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
Expand Down Expand Up @@ -1505,6 +1510,39 @@ public void testLazinessForAuthorizedIndicesSet() {
verify(supplier, never()).get();
}

public void testGetUserPrivilegesThrowsIaeForUnsupportedOperation() {
final RBACAuthorizationInfo authorizationInfo = mock(RBACAuthorizationInfo.class);
final Role role = mock(Role.class);
when(authorizationInfo.getRole()).thenReturn(role);
when(role.cluster()).thenReturn(ClusterPermission.NONE);
when(role.indices()).thenReturn(IndicesPermission.NONE);
when(role.application()).thenReturn(ApplicationPermission.NONE);
when(role.runAs()).thenReturn(RunAsPermission.NONE);

final UnsupportedOperationException unsupportedOperationException = mock(UnsupportedOperationException.class);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: would just instantiate the exception here instead of mocking since we're not relying on any mocked behavior.

switch (randomIntBetween(0, 3)) {
case 0 -> when(role.cluster()).thenThrow(unsupportedOperationException);
case 1 -> when(role.indices()).thenThrow(unsupportedOperationException);
case 2 -> when(role.application()).thenThrow(unsupportedOperationException);
case 3 -> when(role.runAs()).thenThrow(unsupportedOperationException);
default -> throw new IllegalStateException("unknown case number");
}

final IllegalArgumentException e = expectThrows(
IllegalArgumentException.class,
() -> engine.getUserPrivileges(authorizationInfo, new PlainActionFuture<>())
);

assertThat(
e.getMessage(),
equalTo(
"Cannot retrieve privileges for API keys with assigned role descriptors. "
+ "Please use the Get API key information API https://ela.st/es-api-get-api-key"
)
);
assertThat(e.getCause(), sameInstance(unsupportedOperationException));
}

private GetUserPrivilegesResponse.Indices findIndexPrivilege(Set<GetUserPrivilegesResponse.Indices> indices, String name) {
return indices.stream().filter(i -> i.getIndices().contains(name)).findFirst().get();
}
Expand Down