[Roles] Support read-only remote index & cluster sections with read_security access #183126

elena-shostak · 2024-05-10T09:03:01Z

Summary

Changed privilege check from manage_security to read_security, so user with read_security can view remote indexes/remote clusters sections for role.

[Screenshot BEFORE] Role for viewer user with read security before change

[Screenshot AFTER] Role for viewer user with read security after change

Checklist

Unit or functional tests were updated or added to match the most common scenarios

For maintainers

This was checked for breaking API changes and was labeled appropriately

Fixes: #182847

Release note

Changed privilege check from manage_security to read_security, so user with read_security can view remote indexes/remote clusters sections for role.

elena-shostak · 2024-05-10T09:08:15Z

x-pack/plugins/security/server/routes/role_mapping/feature_check.ts

@@ -49,7 +49,7 @@ export function defineRoleMappingFeatureCheckRoute({ router, logger }: RouteDefi
      const esClient = (await context.core).elasticsearch.client;
      const { has_all_requested: canManageRoleMappings } =
        await esClient.asCurrentUser.security.hasPrivileges({
-          body: { cluster: ['manage_security'] },
+          body: { cluster: ['read_security'] },


Role mappings already support read only view https://github.com/elastic/kibana/blob/main/x-pack/plugins/security/public/management/role_mappings/role_mappings_management_app.tsx#L88

Long term, would it make more sense to change this internal endpoint to '/internal/security/_check_security_features'? In that case we would rename 'canManageRoleMappings' to 'canReadSecurity'. We're really using this (all use cases that I can find) to check which features are enabled, with a minimum requirement of "security_read" priv.

Do we know if we have any external (non-Kibana) callers to this API? If not, is it ok to move/change the API now?

cc: @legrego @azasypkin

Long term, would it make more sense to change this internal endpoint to '/internal/security/_check_security_features'? In that case we would rename 'canManageRoleMappings' to 'canReadSecurity'. We're really using this (all use cases that I can find) to check which features are enabled, with a minimum requirement of "security_read" priv.

++ we shouldn't be calling a Role Mappings internal API from the Role Management screen. This isn't a new problem though, it exists in main since the introduction of remote index privileges. I'll leave the "when" up to y'all: I don't have a strong preference if we refactor in this PR, or create a follow-up issue to address as time permits.

Do we know if we have any external (non-Kibana) callers to this API? If not, is it ok to move/change the API now?

We shouldn't have external callers to this API. We reserve the right to change /internal APIs as needed. If we are aware that an /internal API has become defacto public, then it changes the equation a bit, but I think we are safe in this instance.

elena-shostak · 2024-05-10T09:12:20Z

/ci

elena-shostak · 2024-05-10T11:26:25Z

@elasticmachine merge upstream

elena-shostak · 2024-05-13T09:02:36Z

@elasticmachine merge upstream

elena-shostak · 2024-05-13T09:06:33Z

/ci

elasticmachine · 2024-05-13T10:41:27Z

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

x-pack/plugins/security/server/routes/role_mapping/feature_check.ts

…check_security_features

elena-shostak · 2024-05-15T14:56:46Z

@elasticmachine merge upstream

elena-shostak · 2024-05-16T09:14:40Z

@jeramysoucy

As agreed, pushed changes, ready for review:

Moved /internal/security/_check_role_mapping_features to /internal/security/_check_security_features.
Created corresponding API Client SecurityFeaturesAPIClient and moved feature check there. It is now used in RoleMappingsGridPage, EditRoleMappingPage, useFeatureCheck hook for EditRolePage has been updated accordingly.
All tests adjusted accordingly.

cc @legrego @azasypkin

jeramysoucy

LGTM! Just had a few nits and a question.

Thanks for implementing the API refactor! Can we add a note in the PR description that we moved the internal API?

...lugins/security/public/management/role_mappings/edit_role_mapping/edit_role_mapping_page.tsx

...gins/security/public/management/role_mappings/role_mappings_grid/role_mappings_grid_page.tsx

jeramysoucy · 2024-05-16T14:53:02Z

x-pack/plugins/security/server/routes/index.ts

@@ -74,6 +75,7 @@ export function defineRoutes(params: RouteDefinitionParams) {
    defineDeprecationsRoutes(params); // deprecated kibana user roles are not applicable, these HTTP APIs are not needed
    defineIndicesRoutes(params); // the ES privileges form used to help define roles (only consumer) is disabled, so there is no need for these HTTP APIs
    defineRoleMappingRoutes(params); // role mappings are managed internally, based on configurations in control plane, these HTTP APIs are not needed
+    defineSecurityFeatureRoutes(params);


I think it is still ok not to register this endpoint in serverless. But as it is now no longer a role mappings endpoint (never really was), would it benefit us to register it for serverless? Would it simplify the code we had to add to work around needing a "conditional hook" if we could just call the endpoint regardless of build flavor and rely on the response?

cc: @legrego @SiddharthMantri

I think it is still ok not to register this endpoint in serverless.

I agree, it shouldn't be necessary for serverless.

But as it is now no longer a role mappings endpoint (never really was), would it benefit us to register it for serverless? Would it simplify the code we had to add to work around needing a "conditional hook" if we could just call the endpoint regardless of build flavor and rely on the response?

I'll defer to y'all on the complexity tradeoffs. One thing I'm not sure about is if the ES APIs we call in here are all available on Serverless, or if the payloads differ between serverless and stateful. If we enable this endpoint for serverless, then we'll need to validate that it continues to function correctly.

I would keep it out of serverless TBH, at least in scope of that PR since it doesn't change access to the endpoint.
Making endpoint available in serverless changes the scope of this PR a lot then.
Though I'm not sure how much we would leverage with this change, it might require a carefull testing for integration/usage of it and not be that trivial in the end. Seems like the benefit of it might be negligible.

If we consider such change, it would be better to do that in a separate issue/PR.

I would keep it out of serverless TBH, at least in scope of that PR since it doesn't change access to the endpoint.
...
If we consider such change, it would be better to do that in a separate issue/PR.

Sounds good to me. Yeah, I don't think we need to hold up this PR at all. I just wanted to have the discussion.

x-pack/test_serverless/api_integration/test_suites/common/platform_security/role_mappings.ts

kibana-ci · 2024-05-16T17:14:15Z

💚 Build Succeeded

Buildkite Build
Commit: 5be2e9f

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`security`	515	517	+2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`security`	582.0KB	582.4KB	+432.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id	before	after	diff
`security`	68.9KB	69.1KB	+121.0B

Unknown metric groups

async chunk count

id	before	after	diff
`security`	21	22	+1

History

💔 Build #210539 failed 84939b4
💚 Build #210257 succeeded c50ba63
💔 Build #210208 failed ff21423
💔 Build #210163 failed a70419d
💔 Build #210120 failed 7bc6c64
💔 Build #210104 failed 52e4289

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[Roles] Changed privilege check from manage_security to read_security

f71fe49

elena-shostak added bug Fixes for quality problems that affect the customer experience 8.15 candidate Feature:Users/Roles/API Keys Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! labels May 10, 2024

elena-shostak commented May 10, 2024

View reviewed changes

elena-shostak added release_note:fix backport:skip This commit does not require backporting labels May 10, 2024

Merge branch 'main' into 182847-role-read-security

b0306eb

Merge branch 'main' into 182847-role-read-security

2bf3cfd

elena-shostak marked this pull request as ready for review May 13, 2024 10:41

elena-shostak requested a review from a team as a code owner May 13, 2024 10:41

jeramysoucy self-requested a review May 13, 2024 13:41

elena-shostak commented May 14, 2024

View reviewed changes

x-pack/plugins/security/server/routes/role_mapping/feature_check.ts Outdated Show resolved Hide resolved

elena-shostak added 4 commits May 15, 2024 12:55

[Roles] Moved internal/_check_role_mapping_features API to internal/_…

52e4289

…check_security_features

Lint fix

7bc6c64

Tests fix

a70419d

Test fixes

cc69654

kibanamachine and others added 2 commits May 15, 2024 10:56

Merge branch 'main' into 182847-role-read-security

ff21423

Snapshot update

c50ba63

jeramysoucy changed the title ~~[Roles] Role mappings with read_security access~~ [Roles] Support read-only remote index & cluster sections with read_security access May 16, 2024

jeramysoucy requested changes May 16, 2024

View reviewed changes

Fixes

84939b4

Fixes

5be2e9f

jeramysoucy approved these changes May 16, 2024

View reviewed changes

elena-shostak merged commit 3c908b2 into elastic:main May 16, 2024
20 checks passed

kibanamachine added the v8.15.0 label May 16, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Roles] Support read-only remote index & cluster sections with read_security access #183126

[Roles] Support read-only remote index & cluster sections with read_security access #183126

elena-shostak commented May 10, 2024 •

edited

elena-shostak May 10, 2024

jeramysoucy May 14, 2024 •

edited

legrego May 14, 2024

elena-shostak commented May 10, 2024

elena-shostak commented May 10, 2024

elena-shostak commented May 13, 2024

elena-shostak commented May 13, 2024

elasticmachine commented May 13, 2024

elena-shostak commented May 15, 2024

elena-shostak commented May 16, 2024 •

edited

jeramysoucy left a comment

jeramysoucy May 16, 2024

legrego May 16, 2024

elena-shostak May 16, 2024 •

edited

jeramysoucy May 16, 2024

kibana-ci commented May 16, 2024

async chunk count

[Roles] Support read-only remote index & cluster sections with read_security access #183126

[Roles] Support read-only remote index & cluster sections with read_security access #183126

Conversation

elena-shostak commented May 10, 2024 • edited

Summary

Checklist

For maintainers

Release note

elena-shostak May 10, 2024

Choose a reason for hiding this comment

jeramysoucy May 14, 2024 • edited

Choose a reason for hiding this comment

legrego May 14, 2024

Choose a reason for hiding this comment

elena-shostak commented May 10, 2024

elena-shostak commented May 10, 2024

elena-shostak commented May 13, 2024

elena-shostak commented May 13, 2024

elasticmachine commented May 13, 2024

elena-shostak commented May 15, 2024

elena-shostak commented May 16, 2024 • edited

jeramysoucy left a comment

Choose a reason for hiding this comment

jeramysoucy May 16, 2024

Choose a reason for hiding this comment

legrego May 16, 2024

Choose a reason for hiding this comment

elena-shostak May 16, 2024 • edited

Choose a reason for hiding this comment

jeramysoucy May 16, 2024

Choose a reason for hiding this comment

kibana-ci commented May 16, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

Page load bundle

async chunk count

History

elena-shostak commented May 10, 2024 •

edited

jeramysoucy May 14, 2024 •

edited

elena-shostak commented May 16, 2024 •

edited

elena-shostak May 16, 2024 •

edited