Do not fetch reserved roles from native store when Get Role API is called #121971

slobodanadamovic · 2025-02-06T21:54:29Z

The reserved roles are already returned directly from the ReservedRolesStore in TransportGetRolesAction.
There is no need to query and deserialize reserved roles from the .security index just to be merged with static definitions.

…lled The reserved roles are already returned from the `ReservedRolesStore` in `TransportGetRolesAction`. There is no need to query and deserialize reserved roles from the `.security` index when Get Roles API is called.

elasticsearchmachine · 2025-02-06T21:55:03Z

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

slobodanadamovic · 2025-02-07T09:49:41Z

...clusters/src/main/java/org/elasticsearch/test/cluster/local/AbstractLocalClusterFactory.java

-                                    Files.delete(rolesFile);
-                                    Files.copy(distributionDir.resolve("config").resolve("roles.yml"), rolesFile);
-                                    writeRolesFile();
+                                    updateRolesFileAtomically();


This is a fix for Serverless tests.

elasticsearchmachine · 2025-02-07T09:55:19Z

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

slobodanadamovic · 2025-02-07T10:04:24Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

            return;
        }

+        final Set<String> rolesToGet = filterReservedRoleNames(names);


Filtering all roles here to avoid that reserved roles get resolved by NativeRolesStore. The getRoleDescriptors is called from accept method to resolve roles which are later used during authorization.

Hmm I'm not sure that's necessary:

We remove already resolved roles out before handing off to the next resolver in the chain.

So if we are resolving e.g. viewer in stateful, it'll get returned by the reserved role store, and won't get checked by the native role store. Similarly, the file store will come first so any role that gets resolved there won't get resolved by the native role store.

We could make this an assertion here (i.e., no reserved roles ever make it to this point).

Nice catch! I've missed the fact that resolved roles are removed. It makes sense to change it to an assertion.

The filtering is still needed for Serverless, which was the main reason I've added it.
I've introduced it back in the TransportGetRolesAction. I think this is okay given that we already do filtering of reserved roles there. The main change is using the ReservedRoleNameChecker instead of ReservedRolesStore.

Would you mind taking another look?

Yikes, sorry about that. You're totally right -- we hit a different path when fetching by name. Adding the check sounds good to me. Taking a quick look now 👀

slobodanadamovic · 2025-02-07T10:05:41Z

...ity/qa/security-basic/src/javaRestTest/java/org/elasticsearch/xpack/security/GetRolesIT.java

+import static org.elasticsearch.xcontent.XContentFactory.jsonBuilder;
+import static org.hamcrest.Matchers.equalTo;
+
+public class GetRolesIT extends SecurityInBasicRestTestCase {


I've added this integration test mostly for sanity check that GET _security/roles works as expected in Stateful and returns reserved roles as well.

n1v0lg

LGTM -- one suggestion around redundant code.

n1v0lg · 2025-02-14T10:00:30Z

...in/security/src/main/java/org/elasticsearch/xpack/security/authz/store/NativeRolesStore.java

            return;
        }

+        final Set<String> rolesToGet = filterReservedRoleNames(names);


Hmm I'm not sure that's necessary:

We remove already resolved roles out before handing off to the next resolver in the chain.

So if we are resolving e.g. viewer in stateful, it'll get returned by the reserved role store, and won't get checked by the native role store. Similarly, the file store will come first so any role that gets resolved there won't get resolved by the native role store.

We could make this an assertion here (i.e., no reserved roles ever make it to this point).

the native roles store should never be called to resolve reserved built-in roles

…r-reserved-roles-from-get-roles-api

n1v0lg

LGTM!

n1v0lg · 2025-02-17T11:21:52Z

...rity/src/main/java/org/elasticsearch/xpack/security/action/role/TransportGetRolesAction.java

                    if (rd != null) {
                        reservedRoles.add(rd);
-                    } else {
-                        listener.onFailure(new IllegalStateException("unable to obtain reserved role [" + role + "]"));


We're getting rid of this branch because now reservedRoleNameChecker.isReserved(...) can also match on file-based roles in Serverless, right?

That's correct. The failure makes sense as a sanity check when the ReservedRolesStore is the only source of reserved roles. This just is not the case in Serverless.

elasticsearchmachine · 2025-02-17T11:44:15Z

💚 Backport successful

Status	Branch	Result
✅	8.18
✅	9.0

…lled (elastic#121971) The reserved roles are already returned from the `ReservedRolesStore` in `TransportGetRolesAction`. There is no need to query and deserialize reserved roles from the `.security` index just to be merged with "static" definitions.

slobodanadamovic · 2025-02-17T13:04:02Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Questions ?

Please refer to the Backport tool documentation

…lled (elastic#121971) The reserved roles are already returned from the `ReservedRolesStore` in `TransportGetRolesAction`. There is no need to query and deserialize reserved roles from the `.security` index just to be merged with "static" definitions. (cherry picked from commit 5c54c5f)

…lled (#121971) (#122759) The reserved roles are already returned from the `ReservedRolesStore` in `TransportGetRolesAction`. There is no need to query and deserialize reserved roles from the `.security` index just to be merged with "static" definitions. (cherry picked from commit 5c54c5f)

…lled (#121971) (#122753) The reserved roles are already returned from the `ReservedRolesStore` in `TransportGetRolesAction`. There is no need to query and deserialize reserved roles from the `.security` index just to be merged with "static" definitions.

…lled (#121971) (#122752) The reserved roles are already returned from the `ReservedRolesStore` in `TransportGetRolesAction`. There is no need to query and deserialize reserved roles from the `.security` index just to be merged with "static" definitions.

slobodanadamovic added >enhancement :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC Team:Security Meta label for security team auto-backport Automatically create backport pull requests when merged v8.18.1 v9.0.1 labels Feb 6, 2025

slobodanadamovic self-assigned this Feb 6, 2025

elasticsearchmachine added the v9.1.0 label Feb 6, 2025

Update docs/changelog/121971.yaml

db41a40

elasticsearchmachine added the serverless-linked Added by automation, don't add manually label Feb 6, 2025

slobodanadamovic and others added 3 commits February 7, 2025 00:50

filter reserved roles in other cases and add tests

0e5024a

Merge branch 'main' into sa-filter-reserved-roles-from-get-roles-api

b233731

make roles.yml file updates atomic

bc9f4be

slobodanadamovic commented Feb 7, 2025

View reviewed changes

naming nit

f658590

slobodanadamovic requested a review from n1v0lg February 7, 2025 09:52

Merge branch 'main' into sa-filter-reserved-roles-from-get-roles-api

e7e60ee

slobodanadamovic marked this pull request as ready for review February 7, 2025 09:54

slobodanadamovic commented Feb 7, 2025

View reviewed changes

n1v0lg approved these changes Feb 14, 2025

View reviewed changes

slobodanadamovic and others added 6 commits February 14, 2025 11:55

remove unnecessary reserved roles filtering and add an asserition

104339f

Merge branch 'main' into sa-filter-reserved-roles-from-get-roles-api

6907adf

filter reserved roles

ad864c7

fix transport get action tests

9a21687

the native roles store should never be called to resolve reserved built-in roles

use ReservedRoleNameChecker.isReserved consistently

c6c7b51

Merge branch 'main' of github.com:elastic/elasticsearch into sa-filte…

0695590

…r-reserved-roles-from-get-roles-api

slobodanadamovic added 3 commits February 15, 2025 13:25

Merge branch 'main' into sa-filter-reserved-roles-from-get-roles-api

d1e1e01

Merge branch 'main' into sa-filter-reserved-roles-from-get-roles-api

c1e1abc

Merge branch 'main' into sa-filter-reserved-roles-from-get-roles-api

88b710c

slobodanadamovic requested a review from n1v0lg February 17, 2025 09:40

n1v0lg approved these changes Feb 17, 2025

View reviewed changes

slobodanadamovic merged commit 5c54c5f into elastic:main Feb 17, 2025
22 checks passed

This was referenced Feb 17, 2025

[8.18] Do not fetch reserved roles from native store when Get Role API is called (#121971) #122752

Merged

[9.0] Do not fetch reserved roles from native store when Get Role API is called (#121971) #122753

Merged

slobodanadamovic added the v8.19.0 label Feb 17, 2025

slobodanadamovic mentioned this pull request Feb 17, 2025

[8.x] Do not fetch reserved roles from native store when Get Role API is called (#121971) #122759

Merged

Do not fetch reserved roles from native store when Get Role API is called #121971

Do not fetch reserved roles from native store when Get Role API is called #121971

Uh oh!

Conversation

slobodanadamovic commented Feb 6, 2025

Uh oh!

elasticsearchmachine commented Feb 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticsearchmachine commented Feb 7, 2025

Uh oh!

slobodanadamovic Feb 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

slobodanadamovic Feb 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1v0lg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1v0lg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

elasticsearchmachine commented Feb 17, 2025

💚 Backport successful

Uh oh!

slobodanadamovic commented Feb 17, 2025

💚 All backports created successfully

Questions ?

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

slobodanadamovic Feb 7, 2025 •

edited

Loading

slobodanadamovic Feb 14, 2025 •

edited

Loading