elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.58k stars 8.21k forks source link

[SO Tagging] Avoid fetching tags when user doesn't have the permission #160754

Open pgayvallet opened 1 year ago

pgayvallet commented 1 year ago

At the moment, the SO tagging plugin fetches the tags at a fixed interval (15mins by default), for every users, as long as we're authenticated (meaning: not on anonymous pages).

However, to have read permission on the tag type, a user must have access to at least one Kibana feature / application that uses tags (dashboard, visualization...) , or to the savedObjectsManagement feature.

When trying to fetch tags for a user without such permission, an unauthorized error is thrown, and is visible both in the server logs, and in the request's response.

[2023-07-17T16:36:10.786-04:00][ERROR][savedobjects-service.repository.point-in-time-finder] Failed to open PIT for types [tag]

We should avoid such errors, either by:

Ihmo the second option would be the best one (as the third option would not get rid of the error in the logs, given it's thrown from a lower layer)

Technical pointers

pooling mechanism on the client side

https://github.com/elastic/kibana/blob/3730dd0779ed8efd74aee88f57422781ec1ac122/x-pack/plugins/saved_objects_tagging/public/plugin.ts#L71-L84

the route

https://github.com/elastic/kibana/blob/a02c00b8a3e66cc6117aba87b9d64273302f4d29/x-pack/plugins/saved_objects_tagging/server/routes/tags/get_all_tags.ts#L11-L18

the service opening the PIT

https://github.com/elastic/kibana/blob/c6d4fb594b2d4724d8abb1cfff0bbb546d91be2f/x-pack/plugins/saved_objects_tagging/server/services/tags/tags_client.ts#L51-L55

the error that surfaces when we fetch the tags on behalf of a user without the correct permissions

https://github.com/elastic/kibana/blob/94085525a9233423d7b3b0164b56c6213078f20a/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/point_in_time_finder.ts#L130-L133

elasticmachine commented 1 year ago

Pinging @elastic/appex-sharedux (Team:SharedUX)

clintandrewhall commented 1 year ago

@pgayvallet I'm actually a fan of "option one": we should have a server and client-side check for if the person has access to tagging functionality. We'd want to hide the UX, prevent the request, and throw if the API endpoint is hit under those circumstances. Is there something I'm missing?

pgayvallet commented 1 year ago

Is there something I'm missing?

I think the major problem here is that we don't have a way to perform such authz / capabilities check from the client-side (at least that I'm aware of, but @elastic/kibana-security would need to confirm).

legrego commented 1 year ago

However, to have read permission on the tag type, a user must have access to at least one Kibana feature / application that uses tags (dashboard, visualization...) , or to the savedObjectsManagement feature.

Do we consider tags to be a fundamental feature of the platform (like the config & telemetry saved objects), or is there a reason for us to gate access based on individual features? If we treat it like the config object, then we can sidestep this issue altogether by automatically granting access to read tags to all authorized Kibana users, but still gate write access based on the existing privileges.

pgayvallet commented 1 year ago

by automatically granting access to read tags to all authorized Kibana users

@legrego IIRC, we did not do that at the time because the security team (Joe? Brandon?) discouraged us from doing so (but it's been a long time, I could remember wrong).

But yes, I think we can consider tags to be a "platform feature", and granting read access automatically to all users may be a easy solution to solve the problem.

legrego commented 1 year ago

IIRC, we did not do that at the time because the security team (Joe? Brandon?) discouraged us from doing so (but it's been a long time, I could remember wrong).

I unfortunately can't remember either, and I very well may have been one of the voices in this conversation.

But yes, I think we can consider tags to be a "platform feature", and granting read access automatically to all users may be a easy solution to solve the problem.

I don't think that this would pose any meaningful risk, but I'll ping the rest of @elastic/kibana-security for further input

azasypkin commented 1 year ago

I don't think that this would pose any meaningful risk, but I'll ping the rest of @elastic/kibana-security for further input

I can't think of any risk as well. For what it's worth, I tried to dig into the original discussion about the Tagging permission model, but couldn't find anything specific about this topic: https://github.com/elastic/kibana/issues/74571#issuecomment-701019279.

legrego commented 1 year ago

If we move forward with this, the automated privilege grants are maintained within the features plugin. We'd want to add the tags SO type to the read arrays, alongside the config and url types:

https://github.com/elastic/kibana/blob/02a43a1dd027863dc7f8458624e8513c3b5628ca/x-pack/plugins/features/server/feature_registry.ts#L115-L139

ijardillier commented 3 months ago

Any news about this issue ? We have alerting on Kibana errors and this problem often generates alerts :-( Thanks

petrklapka commented 1 month ago

@vadimkibana - are you planning on wrapping this up or can we re-assign to someone else for quick attention?