Limited-by role descriptors in Get/QueryApiKey response #89273

ywangd · 2022-08-11T10:23:12Z

An API key's effective permission is an intersection between its
assigned role descriptors and a snapshot of its owner user's role
descriptors (limited-by role descriptors). In #89166, the assigned
role descriptors are now returned by default in Get/Query API key
responses.

This PR further adds support to optionally return limited-by role
descriptors in the responses. Unlike assign role descriptors,
an API key cannot view any limited-by role descriptors unless it
has manage_api_key or higher privileges.

Relates: #89058

An API key's effective permission is an intersection between its assigned role descriptors and a snapshot of its owner user's role descriptors (limited-by role descriptors). In elastic#89166, the assigned role descriptors are now returned by default in Get/Query API key responses. This PR further adds support to optionally return limited-by role descriptors in the responses. Unlike assign role descriptors, an API key cannot view any limited-by role descriptors unless it has manage_api_key or higher privileges. Relates: elastic#89058

elasticsearchmachine · 2022-08-11T10:23:36Z

Pinging @elastic/es-security (Team:Security)

elasticsearchmachine · 2022-08-11T10:23:36Z

Hi @ywangd, I've created a changelog YAML for you.

ywangd · 2022-08-11T10:26:20Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKey.java

+        String realm,
+        @Nullable Map<String, Object> metadata,
+        @Nullable List<RoleDescriptor> roleDescriptors,
+        @Nullable RoleDescriptorsIntersection limitedBy


I added this new RoleDescriptorsIntersection mainly for future proof (in case we support derived keys properly). It also mirrors what we did with RoleReferenceIntesection and LimitedRole.
It does also introduce some code level overhead. Let me know if you dislike it and prefer a simpler List<RoleDescriptor>.

Missed this comment. RoleDescriptorsIntersection makes sense to me. We know we will need something like this and all the work is already there now 👍

ywangd · 2022-08-11T10:28:39Z

...core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/GetApiKeyRequest.java

+    public static class Builder {
+        private String realmName = null;
+        private String userName = null;
+        private String apiKeyId = null;
+        private String apiKeyName = null;
+        private boolean ownedByAuthenticatedUser = false;
+        private boolean withLimitedBy = false;


Added a builder class and deprecated the various convenient methods. I think this request class has come to a point that a Builder class is justified. I will have a follow-up PR to remove those deprecated methods.

ywangd · 2022-08-11T10:29:43Z

x-pack/plugin/core/src/test/java/org/elasticsearch/test/SecuritySettingsSourceField.java

-          # The _es_test_root role doesn't have any application privileges because that would require loading data (Application Privileges)
-          # from the security index, which can causes problems if the index is not available
+          applications:
+            - application: "*"
+              privileges: [ "*" ]
+              resources: [ "*" ]


This is no longer the case. Added these application privilege so this role definition is consistent with the one used for testclusters.

ywangd · 2022-08-11T10:32:41Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

@@ -2488,6 +2739,7 @@ private void verifyGetResponse(
        List<CreateApiKeyResponse> responses,
        List<Map<String, Object>> metadatas,
        List<RoleDescriptor> expectedRoleDescriptors,
+        List<RoleDescriptor> expectedLimitedRoleDescriptors,


Most changes in this file are due to this method signature change which allows checking the limited-by role descriptors.

n1v0lg

Nice work! Only non-blocking suggestions/questions throughout.

n1v0lg · 2022-08-11T10:58:22Z

...re/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/QueryApiKeyRequest.java

@@ -98,6 +107,10 @@ public void setFilterForCurrentUser() {
        filterForCurrentUser = true;
    }

+    public boolean isWithLimitedBy() {


Nit

Suggested change

public boolean isWithLimitedBy() {

public boolean withLimitedBy() {

I changed it as suggested.

PS: We are not consistently on getter/setter names. In this particular class, the other methods follow JavaBean convention. So I also chose to have isWithLimitedBy here. In GetApiKeyRequest, the existing boolean field follows Builder convention. So I had it differently over there. It is definitely not ideal. We may want to look into larger scale consistency in future.

n1v0lg · 2022-08-11T11:03:40Z

...ck/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/apikey/ApiKey.java

@@ -67,6 +98,9 @@ public ApiKey(
        this.realm = realm;
        this.metadata = metadata == null ? Map.of() : metadata;
        this.roleDescriptors = roleDescriptors;
+        // This assertion will need to be changed (or removed) when derived keys are properly supported
+        assert limitedBy == null || limitedBy.roleDescriptorsList().size() == 1 : "cannot only have one set of limited-by role descriptors";


I think the message should be "can only have one ..."

Absolutely. I changed the wording a few times and they left to be inconsistent ...

n1v0lg · 2022-08-11T11:16:31Z

...e/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptorsIntersection.java

+    private static List<List<RoleDescriptor>> readRoleDescriptorsListFrom(StreamInput in) throws IOException {
+        final List<List<RoleDescriptor>> roleDescriptorsList = new ArrayList<>();
+        final int size = in.readVInt();
+        assert size >= 0;


Nit: would assert false inside the if body below with the same message as that in the exception.

...src/main/java/org/elasticsearch/xpack/security/rest/action/apikey/RestQueryApiKeyAction.java

n1v0lg · 2022-08-11T11:44:18Z

...e/src/main/java/org/elasticsearch/xpack/core/security/authz/RoleDescriptorsIntersection.java

+import java.util.ArrayList;
+import java.util.List;
+
+public record RoleDescriptorsIntersection(List<List<RoleDescriptor>> roleDescriptorsList) implements ToXContentObject, Writeable {


Not for this PR, but I'm wondering if it makes sense to plug this class into CompositeRoleStore.getRoleDescriptorsList -- there's a type mismatch (Collection<Set<>> vs List<List<>>), but semantically, the result of getRoleDescriptorsList seems to be exactly a RoleDescriptorsIntersection.

In the future, if we support non-singleton limited-by role descriptor intersections we might also want to use RoleDescriptorsIntersection through ApiKeyService (e.g., as an arg to createApiKey) but that's a refactor for another day.

WDYT?

I changed the type to be Collection<Set<>> so it is consistent.

That said, I cannot remember why existing code uses Collection instead of List. It does not seem necessary. I used List for the inner type instead of Set in the hope to keep a predicatable order. But that was not really useful because these information is rendered as map on the user-facing output. Also we deduplicate role names when resolving role definitions. So overall Set is a better fit for the inner type. In summary, I think the better type would be List<Set<>> but this can wait for future PRs.

Excellent and +1 on List<Set<>>

n1v0lg · 2022-08-11T15:46:18Z

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

@@ -207,7 +207,8 @@ static boolean checkSameUserPermissions(String action, TransportRequest request,
                    // if the authentication is an API key then the request must also contain same API key id
                    String authenticatedApiKeyId = (String) authentication.getMetadata().get(AuthenticationField.API_KEY_ID_KEY);
                    if (Strings.hasText(getApiKeyRequest.getApiKeyId())) {
-                        return getApiKeyRequest.getApiKeyId().equals(authenticatedApiKeyId);
+                        // An API key with no manage_api_key or higher is not allowed to view its own limited-by role descriptors
+                        return getApiKeyRequest.getApiKeyId().equals(authenticatedApiKeyId) && false == getApiKeyRequest.withLimitedBy();


This one was nice and hidden away 😅

n1v0lg · 2022-08-11T16:09:10Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

-        final GetApiKeyResponse getApiKeyResponse = future.actionGet();
-        assertThat(getApiKeyResponse.getApiKeyInfos(), arrayWithSize(1));
-        final ApiKey apiKeyInfo = getApiKeyResponse.getApiKeyInfos()[0];
+        final ApiKey apiKeyInfo;


I like the "free" coverage this gives us but a little hesitant to sneak more implicit testing into this method since it's primarily meant for asserting on expectations for other tests. There's already a lot going on in here; adding another layer of randomization and implicit testing might make debugging test issues/maintenance cost too heavy. I'm good to keep this as is but if you had the same hesitation could be worth iterating.

Alternatively, at least renaming to something like doTestApiKeyHasExpectedAttributes would better match what's in the box

I renamed it doTestApiKeyHasExpectedAttributes. I'd like to keep the "free" coverge. But I also think your point of "another layer of randomization and implicit testing might make debugging test issues/maintenance cost too heavy" is valid. As a middle ground, I added a message in all relevant assertion to say which API is randomised (Get or Query). So this at least should give us some hint on which part went wrong on failure.

We can definitely still improve on the tests in future PRs. There are many things that can be cleaned up or better organised.

n1v0lg · 2022-08-11T16:37:39Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

+                        containsInAnyOrder(expectedLimitedRoleDescriptors.toArray(RoleDescriptor[]::new))
+                    );
+                }
+            }


Could add something like to make it more explicit that we don't assert on expectedLimitedRoleDescriptors correctly when there's more than one user passed in:

} else { assertNull("can only assert on limited_by for single user, must pass null otherwise", expectedLimitedRoleDescriptors); }

Alternatively (this would be a bigger change - not necessary in this PR, and possibly not worth it) could pass a map of username to expected limitedBy RDs, e.g. something like:

Map.of( ES_TEST_ROOT_USER, List.of(ES_TEST_ROOT_ROLE_DESCRIPTOR), "user_with_manage_api_key_role", List.of(...) )

I added the null assertion.

pass a map of username to expected limitedBy RDs

I considered that too but deciced not to do so because this multiple user assertion is only used in one place which does not really justify the cascading changes. There is definitely refactoring opportunities. But I'll leave it for now.

On a second thought, I refactored this method to take lookup function to get expectedLimitedByRoleDescriptors for different users. This is because other changes such as randomisation (as suggested below) has made cascading changes no longer a valid concern. It is also good to get rid of the special handling inside this method.

n1v0lg · 2022-08-11T16:57:14Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

+                    );
+                }
+            }
+        });
    }



Doesn't have to be in this PR but would be good to add a test that covers getting API keys with legacy superuser role descriptors (i.e., that they will automatically be mapped to non-legacy ones).

I meant to test that but somehow forgot. Thanks for catching it. Added a new test method testLegacySuperuserLimitedByWillBeReturnedAsTransformed

n1v0lg · 2022-08-11T17:19:52Z

...ty/src/internalClusterTest/java/org/elasticsearch/xpack/security/authc/ApiKeyIntegTests.java

@@ -1171,6 +1303,92 @@ public void testApiKeyAuthorizationApiKeyMustBeAbleToRetrieveItsOwnInformationBu
        assertErrorMessage(ese, "cluster:admin/xpack/security/api_key/get", ES_TEST_ROOT_USER, responses.get(0).getId());
    }

+    public void testApiKeyViewLimitedBy() {


Optional, but wondering if we should randomize between query and get in this method -- would give a nice single place where we test both ways to view limited by

Yep, added the randomisation.

A builder class for GetApiKeyRequest is added as part of #89273. As a result, the existing convenient methods got deprecated in favour of the builder. This PR removes the deprecated methods and replaces all usages with the builder.

This PR updates relevant docs and yaml tests to cover the new feature of viewing API key's limited-by role descriptors introduced in #89273 Relates: #89058

ywangd added >enhancement :Security/Security Security issues without another label v8.5.0 labels Aug 11, 2022

ywangd requested a review from n1v0lg August 11, 2022 10:23

elasticsearchmachine added the Team:Security Meta label for security team label Aug 11, 2022

Update docs/changelog/89273.yaml

7d1cc03

ywangd commented Aug 11, 2022

View reviewed changes

n1v0lg approved these changes Aug 11, 2022

View reviewed changes

address feedback

0cfa262

ywangd added the auto-merge-without-approval Automatically merge pull request when CI checks pass (NB doesn't wait for reviews!) label Aug 12, 2022

elasticsearchmachine merged commit 8dfbcd5 into elastic:main Aug 12, 2022

ywangd deleted the view-limited-by-role-descriptors branch August 12, 2022 06:18

ywangd mentioned this pull request Aug 16, 2022

Remove deprecated convenient methods from GetApiKeyRequest #89360

Merged

ywangd mentioned this pull request Aug 18, 2022

Docs and yaml tests for viewing API key's limited-by #89443

Merged

n1v0lg mentioned this pull request Nov 24, 2022

Remote access authentication header serialization #91816

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Limited-by role descriptors in Get/QueryApiKey response #89273

Limited-by role descriptors in Get/QueryApiKey response #89273

ywangd commented Aug 11, 2022

elasticsearchmachine commented Aug 11, 2022

elasticsearchmachine commented Aug 11, 2022

ywangd Aug 11, 2022

n1v0lg Aug 11, 2022

ywangd Aug 11, 2022

ywangd Aug 11, 2022

ywangd Aug 11, 2022

n1v0lg left a comment

n1v0lg Aug 11, 2022

ywangd Aug 12, 2022

n1v0lg Aug 11, 2022

ywangd Aug 12, 2022

n1v0lg Aug 11, 2022

n1v0lg Aug 11, 2022

ywangd Aug 12, 2022

n1v0lg Aug 12, 2022

n1v0lg Aug 11, 2022

n1v0lg Aug 11, 2022

n1v0lg Aug 11, 2022

ywangd Aug 12, 2022

n1v0lg Aug 11, 2022

n1v0lg Aug 11, 2022

ywangd Aug 12, 2022

ywangd Aug 12, 2022

n1v0lg Aug 11, 2022

ywangd Aug 12, 2022 •

edited

Loading

n1v0lg Aug 11, 2022

ywangd Aug 12, 2022

	public boolean isWithLimitedBy() {
	public boolean withLimitedBy() {

Limited-by role descriptors in Get/QueryApiKey response #89273

Limited-by role descriptors in Get/QueryApiKey response #89273

Conversation

ywangd commented Aug 11, 2022

elasticsearchmachine commented Aug 11, 2022

elasticsearchmachine commented Aug 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

n1v0lg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd Aug 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ywangd Aug 12, 2022 •

edited

Loading