-
Notifications
You must be signed in to change notification settings - Fork 8.1k
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
[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
@@ -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'] }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/ci |
@elasticmachine merge upstream |
@elasticmachine merge upstream |
/ci |
Pinging @elastic/kibana-security (Team:Security) |
x-pack/plugins/security/server/routes/role_mapping/feature_check.ts
Outdated
Show resolved
Hide resolved
…check_security_features
@elasticmachine merge upstream |
As agreed, pushed changes, ready for review:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
...gins/security/public/management/role_mappings/role_mappings_grid/role_mappings_grid_page.tsx
Outdated
Show resolved
Hide resolved
@@ -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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
History
To update your PR or re-run it, just comment with: |
Summary
Changed privilege check from
manage_security
toread_security
, so user withread_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
For maintainers
Fixes: #182847
Release note
Changed privilege check from
manage_security
toread_security
, so user withread_security
can view remote indexes/remote clusters sections for role.