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.

Sign up for GitHub

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

Jump to bottom

[Infra][ECO] Fix RBAC issue in hosts view #199841

Merged
merged 8 commits into from
Nov 21, 2024
Merged

[Infra][ECO] Fix RBAC issue in hosts view #199841

merged 8 commits into from
Nov 21, 2024

Conversation

crespocarlos
Copy link
Contributor

@crespocarlos crespocarlos commented Nov 12, 2024 •
edited by kibanamachine
Loading

closes #200151

Summary

This PR change the getApmIndices function from apm_data_access to fetch the information using Kibana's internal user. This was done for 2 reasons:

1 - Plugins using savedObjects.client might face a situation where the logged in user doesn't have permission to read saved objects, causing the retrieval of apm indices to fail, which could lead to unexpected exceptions
2 - Elasticsearch is able to determine whether the user has permission to view docs in the index patterns, therefore, it's ok to retrieve the index pattern with Kibana's internal user because ultimately elasticsearch will only return the data the user has access to.

Infra app permission

Role config:

image image

Without access to APM indices
image

image

With access to APM indices

image image

Admin

image

How to test

  • Follow the steps above
  • Other areas affected: assistant and profiling

@crespocarlos
Copy link
Contributor Author

/ci

@crespocarlos crespocarlos force-pushed the fix-rbac-hosts-view branch 2 times, most recently from 739a20c to 19f0e7c Compare November 13, 2024 18:01
@crespocarlos
Copy link
Contributor Author

/ci

@crespocarlos
Copy link
Contributor Author

/ci

@crespocarlos
Copy link
Contributor Author

/ci

@crespocarlos
Copy link
Contributor Author

/ci

@crespocarlos crespocarlos added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Nov 18, 2024
@crespocarlos crespocarlos marked this pull request as ready for review November 18, 2024 09:12
@crespocarlos crespocarlos requested review from a team as code owners November 18, 2024 09:12
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services)

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@botelastic botelastic bot added the ci:project-deploy-observability Create an Observability project label Nov 18, 2024
@github-actions GitHub Actions
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

@jennypavlova jennypavlova self-requested a review November 18, 2024 11:40
jennypavlova
jennypavlova approved these changes Nov 18, 2024
View reviewed changes
Copy link
Member

@jennypavlova jennypavlova left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The code LGTM 🚀
One security concern: I am not sure if that's related to the change but if the user is restricted to read everything in observability except APM and has restricted indices he is still able to see the services in the details view (the link won't navigate to APM because the user doesn't have the permission - it will be Application not found) but should we still show the services section/name in the host view if we set that restriction (no APM UI/Indecies access)?
The role I created to test this case:
image

no_apm_permission.mov

This could be something to address separately from this PR

@crespocarlos
Copy link
Contributor Author

crespocarlos commented Nov 18, 2024 •
edited
Loading

The code LGTM 🚀 One security concern: I am not sure if that's related to the change but if the user is restricted to read everything in observability except APM and has restricted indices he is still able to see the services in the details view (the link won't navigate to APM because the user doesn't have the permission - it will be Application not found) but should we still show the services section/name in the host view if we set that restriction (no APM UI/Indecies access)?

This could be something to address separately from this PR

Hey @jennypavlova, great finding. In such cases we should IMO show the services but not link to APM. Users can still query for APM data in Discover, so the data is not restricted. What is restricted in your scenario is the access to APM app.

edit: let's open a new ticket for this problem

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

@crespocarlos
Copy link
Contributor Author

@elasticmachine merge upstream

rudolf
rudolf approved these changes Nov 20, 2024
View reviewed changes
Copy link
Contributor

@rudolf rudolf left a comment

Choose a reason for hiding this comment

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

Changes to x-pack/plugins/observability_solution/apm_data_access/kibana.jsonc lgtm

@elasticmachine
Copy link
Contributor

elasticmachine commented Nov 21, 2024 •
edited
Loading

💚 Build Succeeded

  • Buildkite Build
  • Commit: e8903b2
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-199841-e8903b2b3be2

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
apmDataAccess 93 86 -7
Unknown metric groups

API count

id before after diff
apmDataAccess 93 86 -7

ESLint disabled line counts

id before after diff
apmDataAccess 0 1 +1

Total ESLint disabled count

id before after diff
apmDataAccess 1 2 +1

History

@crespocarlos crespocarlos merged commit 209c667 into elastic:main Nov 21, 2024
26 checks passed
@crespocarlos crespocarlos deleted the fix-rbac-hosts-view branch November 21, 2024 12:06
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11952856046

@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
❌ 8.x Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 199841

Questions ?

Please refer to the Backport tool documentation

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 22, 2024
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199841 locally

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199841 locally

paulinashakirova pushed a commit to paulinashakirova/kibana that referenced this pull request Nov 26, 2024
@crespocarlos @kibanamachine
@elasticmachine @paulinashakirova
[Infra][ECO] Fix RBAC issue in hosts view (elastic#199841)
db85cb3 
closes [elastic#200151](elastic#200151)

## Summary

This PR change the `getApmIndices` function from `apm_data_access` to
fetch the information using Kibana's internal user. This was done for 2
reasons:

1 - Plugins using `savedObjects.client` might face a situation where the
logged in user doesn't have permission to read saved objects, causing
the retrieval of apm indices to fail, which could lead to unexpected
exceptions
2 - Elasticsearch is able to determine whether the user has permission
to view docs in the index patterns, therefore, it's ok to retrieve the
index pattern with Kibana's internal user because ultimately
elasticsearch will only return the data the user has access to.

### Infra app permission

**Role config:**

<img width="500" alt="image"
src="https://github.com/user-attachments/assets/ae98a98f-570a-4139-b804-91a8de0c9d1d">

<img width="500" alt="image"
src="https://github.com/user-attachments/assets/afe29e7f-ab02-40f4-a86c-aeb016655708">


**Without access to APM indices**
<img width="500" alt="image"
src="https://github.com/user-attachments/assets/8aa7d4e5-3484-4723-838c-54920e442c08">

<img width="500" alt="image"
src="https://github.com/user-attachments/assets/af3ce400-7a45-4313-84c7-5b8170c39bf5">

**With access to APM indices**

<img width="500" alt="image"
src="https://github.com/user-attachments/assets/1effc137-72a2-4e5b-b2ac-62e685370a21">

<img width="500" alt="image"
src="https://github.com/user-attachments/assets/e161f6d9-85a7-4f80-a7d3-7ec0bdc338a3">


### Admin

<img width="500" alt="image"
src="https://github.com/user-attachments/assets/d280f0d6-de6c-408f-a080-fa150d237afc">


### How to test

- Follow the steps above
- Other areas affected: assistant and profiling

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199841 locally

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199841 locally

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 199841 locally

@cauemarcondes cauemarcondes added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 1, 2024
cauemarcondes added a commit that referenced this pull request Dec 2, 2024
@cauemarcondes
Revert "[Infra][ECO] Fix RBAC issue in hosts view (#199841)"
3839247 
This reverts commit 209c667.
This was referenced Dec 2, 2024
Merged
Open
cauemarcondes added a commit that referenced this pull request Dec 2, 2024
@cauemarcondes
Revert "[Infra][ECO] Fix RBAC issue in hosts view" (#202418)
99463c7 
Reverts #199841
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jennypavlova jennypavlova jennypavlova approved these changes

@klacabane klacabane klacabane approved these changes

@rudolf rudolf rudolf approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Infra] Hosts View fails when user doesn't have permission to read Saved Objects
7 participants
@crespocarlos @elasticmachine @kibanamachine @rudolf @klacabane @jennypavlova @cauemarcondes