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

Dashboard Builder: Add capability to use Grafonnet #13

Merged
merged 1 commit into from
Apr 26, 2024
Merged

Conversation

amotl
Copy link
Contributor

@amotl amotl commented Apr 25, 2024

About

In order to expand GH-10, this patch adds the capability to use Grafonnet as dashboard builder.

References

Copy link
Collaborator

@peekjef72 peekjef72 Apr 26, 2024

Choose a reason for hiding this comment

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

I'm wondering about the security of calling a command line without checking its content ?
Should it increase user privileges ? if the command (grafana-import) has sudo rights it may cause pb !
It is especially for .py files (not jsonnet !)
Maybe a warning on this case should be set in the README...
It is a marginal case but...

Copy link
Contributor Author

@amotl amotl Apr 26, 2024

Choose a reason for hiding this comment

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

Hi. I hear you on one hand, but on the other, giving a regular program like grafana-import which can do all sorts of things root permissions would be a bad idea per se, no?

If a user can run a (dashboard builder) program with python dashboard-builder.py, or if they do it through grafana-import, does not make any big difference, right?

It might be different when grafana-import would expose its functionality via a web service or similar means, where the user and the runtime are on different machines. But still in this case, I guess shlex.split() offers enough protective measures so it can't be exploited to invoke a different program, if you are referring to that?

So, if you don't mind, my personal evaluation of this is that shelling out like grafana-import is doing it, is safe enough for its purpose. If you really think it's not, or if anyone else from the community has any other objections or concerns, please let me know on behalf of a concrete example that demonstrates a relevant vulnerability, by creating a dedicated issue.

if the command (grafana-import) has sudo rights it may cause pb !

If you think I misunderstood your statement, where I translated "sudo rights" into "root permissions", please let me know. What does "pb" actually refer to?

Base automatically changed from reload to main April 26, 2024 21:41
@amotl amotl merged commit 1489fe3 into main Apr 26, 2024
2 checks passed
@amotl amotl deleted the grafonnet branch April 26, 2024 21:42
@peekjef72
Copy link
Collaborator

peekjef72 commented Apr 27, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants