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.

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

Enhanced security model / actual ReadOnly permission level #82950

Open
gelicia opened this issue Feb 16, 2024 Discussed in #43487 · 5 comments
Open

Enhanced security model / actual ReadOnly permission level #82950

gelicia opened this issue Feb 16, 2024 Discussed in #43487 · 5 comments

Comments

@gelicia
Copy link
Contributor

gelicia commented Feb 16, 2024

Discussed in #43487

Originally posted by ppolewicz December 22, 2021

What would you like to be added

There is a pair of issues that are impractical to be discussed separately:

  1. Grafana does not validate the request parameters (discussed in quite a few issues over the years, but grafana does not validate variables in url #18157 seems to summarize it neatly)
  2. Grafana allows the Viewer user to make any database query and not only those defined by dashboards and panels (reported in many places, but IMHO described best in this comment)

The addition would be some kind of an OPTIONAL setting that would allow to use Grafana in a "really read only" mode, where only the queries written by the Dashboard designer can be executed by the user and only with parameters allowed by the Dashboard designer.

Why is this needed (describe your use case and goals)

For some installations it is impractical to separate the data into different databases for different users and being able to restrict who sees what is desired. There might be, I guess, a ton of issues related to the user being able to interact with a database directly. Those might not be officially classified as security today because the security model considers the Viewer user to be authorized to make any query to the database and but I don't want my service to be DoSed by a kid.

How?

The basic design is described here, @torkelo confirmed that it is possible to implement it, though some restrictions might apply. That's fair and fine - depending on a setting, enhanced security might provide a more rigid structure in which not everything can be as easily achieveable as with the open model. We do expect that a dashboard with parameters filled with timeseries will work fine though.

A more detailed plan is provided here. It looks like the code needs to be modified in four places, though some of those are as simple as adding a config flag, or an if statement doing basic input validation (checking membership in a list etc).

Alternatives

Instead of a config flag, something else could be used - permission level lower than the current Viewer, "Basic" or something like that. Users with that level of access would have their inputs validated and would not be permitted to query the database directly, only via the new "queryless" endpoint. In fact I like that model more and perhaps eventually we should move towards it, but initially I'd like the new behavior to be hidden behind a global config option so that early adopters can report any issues before the feature is visible for everyone. I guess the flag could enable setting "Basic" as the user access level, to reduce the scope of the migration later on.

The details on how to expose it to the users are not firm, bu the core of the idea is simple.

Who

I would implement it with a couple of friends also bothered / impacted by this issue. What we are asking for today is a design review and preliminary approval of the implementation plan from core grafana developers, then code review and merging when we finish the implementation.

@septatrix
Copy link

Cross-referencing also issues #32043 and #21856 which cover the same or at least conceptually similar issues. Those issues should also be taken into account when implementing this feature as it seems likely that they can be solved together (or at least build upon each other).

@torkelo
Copy link
Member

torkelo commented Feb 23, 2024

@gelicia work to support this was started by the public data source work, which issues all queries from the backend (via only sending dashboard uid and panelId from frontend). Biggest limitation it does not support variables. And many dynamic dashboard features that leverage variables.

Supporting variable queries and substitution in a fully secure way is next to impossible given how flexible and unrestricted variables can be (can represent any query part). But should still provide some value. Anyway implementing the full variable query system with support for lazy and nested variables, and the full interpolation engine in the backend is a very big task.

I think we have some design docs for this already.

@gelicia
Copy link
Contributor Author

gelicia commented Feb 23, 2024

@ppolewicz This issue was migrated from your discussion, and @torkelo has provided some additional information on progress. Just wanted to be sure you were kept in the loop :)

@gelicia
Copy link
Contributor Author

gelicia commented Feb 23, 2024

Also @Kn0rk3 as well - thank you for your patience on the migration!

@ppolewicz
Copy link

Cool.

I think implementing a solution that works for some variables and can be extended for more in the future is better than not implementing anything at all. I use it as a map that uses strings from a limited number of values (client name, cluster name, instance name + time range). That should be very doable.

@torkelo can I see that design doc?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: 🗂️ Needs Triage / Escalation
Development

No branches or pull requests

4 participants