CLI: Allow installing custom binary plugins #17551

aocenas · 2019-06-12T15:23:18Z

This is Grafana side of things for https://github.com/raintank/grafana-net/pull/790
Basically it works the same as before, ask grafana.com API for plugin, get a version and then download it via the same API. Main things this add:

Check versions for whether they are actually supported for the os/arch the grafana is running on.
Check whether the expected file checksum matches the downloaded archive
Handle symlinks when decompressing the zip archive. (this is needed for Chromium to be able to work after unzipping)

There is an issue with how to handle the decompressing properly. Symlinks can create an issue but I think only in case the privileges would be elevated when running. For example install it as user, symlink to some file you do not have access to, the run as root and read the symlink. This is not happening and only issue could be when we scan for plugin.json which we do recursively while following symlinks but I think this should be changed so it goes only 2 level down (see comment in the code). But this could be an issue in the future.

Another issue is probably this https://snyk.io/research/zip-slip-vulnerability. Basically you can unzip files into destination that are outside of the plugin directory. Right now we do not check that in any way. This though is probably at the moment problem only when installing something from custom url. Installing plugin through grafana api installs it only via github source zipball and so I do not think it is possible to generate malicious zip that way. There is popular library that handles archiving and they do not handle this:

Security note: This package does NOT attempt to mitigate zip-slip attacks. It is extremely difficult to do properly and seemingly impossible to mitigate effectively across platforms. Attempted fixes have broken processing of legitimate files in production, rendering the program unusable. Our recommendation instead is to inspect the contents of an untrusted archive before extracting it (this package provides Walkers) and decide if you want to proceed with extraction.

Update:
The final version of the code checks for a zip-slip issue directly. Not considering symlinks this seems to be easy. Symlinks are only allowed for internal Grafana plugins (as it is needed only for the renderer and the bundled Chromium right now).

Ref #18083

TODO:

other plugins command like list
tests

pkg/cmd/grafana-cli/commands/install_command.go

aocenas · 2019-06-12T15:26:48Z

pkg/cmd/grafana-cli/utils/grafana_path.go

@@ -6,35 +6,51 @@ import (
 	"path/filepath"


Some fixes here because after last change, you could run grafana-cli from repo root, it would install the plugin to wrong directory.

aocenas · 2019-06-12T15:27:44Z

pkg/plugins/plugins.go

@@ -164,11 +164,14 @@ func scan(pluginDir string) error {
 }

 func (scanner *PluginScanner) walker(currentPath string, f os.FileInfo, err error) error {
+	// TODO: Based on how we search for plugin.json in grafan-net fetchPluginJson, it should be enough to scan
+	// just /src, /dist or / paths inside the plugin dir


Not sure if having plugin.json anywhere in the package is something we want to support, it is not something we support for packages registered in grafana.com API

Not sure if having plugin.json anywhere in the package is something we want to support

Based on https://grafana.com/docs/plugins/developing/code-styleguide/#plugin-json-mandatory I would say that we shouldn't.

The problem is dealing with plugins that embed other plugins, like https://github.com/raintank/worldping-app/tree/master/dist/grafana-worldmap-panel

Within an individual plugin we expect plugin.json to be in those locations, but we walk all the way down through the plugin to find any embedded plugins.

aocenas · 2019-06-12T15:28:32Z

pkg/plugins/plugins.go

 	if err != nil {
 		return err
 	}

-	if f.Name() == "node_modules" {
+	if f.Name() == "node_modules" || f.Name() == "Chromium.app" {


Not liking this hardcoding here like this. If we allowed just one level of traversal it could probably just go away.

aocenas · 2019-06-18T12:32:39Z

pkg/cmd/grafana-cli/commands/install_command.go

-	}()
-
-	resp, err := http.Get(url) // #nosec
-	if err != nil {


Bit of an issue in this old code. This does not check for response status. So even though for old versions this will return correctly 400 status because we do not know what is the arch of the grafana it will end up with:

zip: not a valid zip file

which is not helpful. Not sure how to make this better.

@torkelo what is your opinion on this? Other solution could be not even return the arch specific plugin from the API if we do not sent arch from grafana (so old versions) in that case user would get plugin does not exists or something like that which does not seem much better and at the same time I think the API would be weirder and non standard (ie if you GET /plugins you would get just a subset and there wouldn't be a clear way to get all plugins no matter what arch, would need to add something like /plugins?all=true or something like that)

Not sure I understand fully. So new grafana-cli adds arch as parameter, the change to grafana.com api is that backwards compatible for non backend plugins (it will need to be). ?

So this is an error for old grafana versions trying to install a new grafana backend plugin? hard to modify that error message now I think.

@torkelo
Yes I think you get it but just to reiterate:

GET /plugins will return all plugins with arch attribute and it is up to the client to filter them correctly

old grafana does not do anything so shows all plugins (so cli plugins list-remote will show everything without knowing which can be installed)

new grafana will show only available

GET /plugins/:id/downloads has a new optional param osAndArch. If set it will return the zip file for specified arch, if not set it will return zip only if frontend plugin or if there is any arch zip file

old grafana can download non backend plugins as usual, for backend plugins it will error out with zip: not a valid zip file because it does not check for response status and we cannot know what arch the grafana is running on

new grafana sends the osAndArch so we pick the correct version (TODO I need to add the status check in the code) and does not allow you to install wrong plugin version with some sensible error message

So adding osAndArch to GET /plugins in a way that without it, it will return only plugins which can be safely installed when we do not know the arch would be best backward compatible way, but not sure if that does not break other things that would expect it to return all plugins by default. It is also kind of nonintuitive behaviour.
Other solution would be to start versioning the grafana.com API on some level (either with some header or v2 prefix maybe) if we would consider that a breaking API change. That would make doing changes a bit easier in sense of keeping backward compatibility but probably harder for maintenance.

I wish we could limit it to new versions but we cant really do that since we offer premium plugins for olders versions of Grafana.

So @DanCech pointed out to me here that we are actually sending OS and ARCH already so it seems it would make sense to reuse those headers. Unfortunately for this issue it just complicates things a bit.

We only send the headers in the /plugin/:id request that gets the metadata but not in the /download so we could know that plugin supports the grafana os/arch but than we still cannot select the correct version to download.

In the /plugin/:id we won't be able to say if it is GF version before the change or after because the headers will be sent also from the old versions.

I thought we could just base it on raw grafana version that we also send in the headers but it seems like it is hardcoded to master 7462577#diff-1146cad1c61c159a07758ade40c6ac2aR12 and does not seem to change since (I assume we forgot to add it to release guidelines that it needs to be updated or something like that)

That variable is set using a compile flag in build.go

Not sure i understand the problem can’t we add these headers to the download call as well. ?

@torkelo

That variable is set using a compile flag in build.go

Ah ok then it can be used.

Not sure i understand the problem can’t we add these headers to the download call as well. ?

Yes I will add this. The only issue is old versions of Grafana before the change how will they handle installing plugins and what would happen if they try to install binary plugin. As I mentioned at the moment they will return zip: not a valid zip file error which is not very helpful. With the grafana version sent, we can return some more helpful error message on the /plugin/:id call for the old Grafana versions. This would mean minimal changes to the API but still old Grafana will get You need to upgrade your Grafana version to install this plugin error or something like that.

Ah I see. If you can find a way to show error to older Grafana versions that be great.

Maybe you can do that by checking the grafana dependency version plugins specify in plugin.json. This is a big missing thing we have yet to fix, that is return info error message when you try to install a plugin or plugin version that is not compatible with your Grafana version

bergquist · 2019-06-19T14:27:43Z

pkg/cmd/grafana-cli/commands/install_command.go

 	if downloadURL == "" {
-		plugin, err := s.GetPlugin(pluginName, c.RepoDirectory())
+		if strings.HasPrefix(pluginName, "grafana-") {


Good enough for now. Any thoughts about how we can sign plugins and verify them in Grafana?

Interesting thought but I am not familiar with these kind of things that much. Only experience I have is iOS app signing and at least the dev experience adds a some unnecessary (I think) complexity. There it is done I think mainly so that you cannot install apps from unsupported places (outside of App store) but if we want to still allow installation of non signed third party plugins not sure if the signing will have that much added value.

We should add signing to our ecosystem for sure. We can use that for whatever we need. Perhaps: automatically trust anything signed by grafana and a big big warnings when installing stuff signed by other people, and even bigger warnings when not signed at all?

Well the question is what is the end goal of that. I mean right now what you can trust is anything that comes from our plugin API I assume. If we have signed plugins I assume you could distribute it by other means and still be able to decide whether to trust it based on whether it is signed, that I assume would allow something like a third party plugin API with trusted plugins but not sure that is important for us.

I am not against that or anything I am just genuinely curious what are the benefits because from my very modest understanding of the topic I do not see that many.

bergquist · 2019-06-19T14:29:49Z

pkg/cmd/grafana-cli/commands/install_command.go

+	if latestForArch.Version == ver.Version {
+		return ver, nil
+	}
+	return nil, xerrors.Errorf("Version you want is not supported on your architecture. Latest suitable version is %v", latestForArch.Version)


Perhaps we should create a helper function for this in errutil?

just to avoid taking deps on the current xerror path.

In the future this should be changed to errors and I think it should no be much work to just do s/xerrors/errors globally. Aliasing everything behind errutils whether transiently or forever does not seem worth it to me, either we will need to rename it later too or we have alias for otherwise standard go package which can be confusing.

bergquist · 2019-06-19T14:42:20Z

pkg/cmd/grafana-cli/commands/install_command.go

-	}()
-
-	resp, err := http.Get(url) // #nosec
-	if err != nil {


I wish we could limit it to new versions but we cant really do that since we offer premium plugins for olders versions of Grafana.

pkg/cmd/grafana-cli/commands/install_command.go

bergquist · 2019-06-19T14:45:34Z

pkg/cmd/grafana-cli/services/api_client.go

+func (client *GrafanaComClient) DownloadFile(pluginName, filePath, url string, checksum string) (content []byte, err error) {
+	client.retryCount = 0
+
+	defer func() {


bergquist · 2019-06-19T14:47:40Z

pkg/cmd/grafana-cli/utils/grafana_path.go

 )

 func GetGrafanaPluginDir(currentOS string) string {
 	if isDevEnvironment() {
-		return "../data/plugins"
+		rootPath, err := getGrafanaRoot()


@gotjosh @aocenas make sure to find consensus about how to solve the path issues before merging this

Taking a look at this PR is on my things to do - sadly, it's quite a heavy change so it's going to take me some time.

I'll try and find some today.

Seems like the work @gotjosh is doing goes in better direction. This is more of a quick fix (maybe for separate PR) but it does not seem to directly clash, just something that will be overwritten anyway later on I assume.

marefr

Great work! Code wise looking really good and very nice tests 👍

Have added some comments where some of them needs to be fixed and some others is just nit picking. We can you through them together if you want? Haven't done any manual tests yet, only code review.

pkg/cmd/grafana-cli/commands/install_command.go

pkg/cmd/grafana-cli/commands/install_command_test.go

pkg/cmd/grafana-cli/services/api_client.go

pkg/cmd/grafana-cli/utils/grafana_path.go

pkg/services/rendering/rendering.go

marefr

Code wise looks great so approving this 👍 But maybe need manual testing before merge?

* grafana/master: Plugins: return a promise for loadPluginCss (grafana#18273) Utils: avoid calling console.warn() too often for deprecation warnings (grafana#18269) CLI: Allow installing custom binary plugins (grafana#17551) Docs: Update link to example app (grafana#18253) GettingStarted: Skip Query for getting started (grafana#18268)

* grafana/master: Chore: noImplicitAny Sub 500 errors (#18287) Plugins: return a promise for loadPluginCss (#18273) Utils: avoid calling console.warn() too often for deprecation warnings (#18269) CLI: Allow installing custom binary plugins (#17551) Docs: Update link to example app (#18253) GettingStarted: Skip Query for getting started (#18268) v6.3.0-beta2 is latest testing (#18283) Release: Changelog update with v6.3.0-beta2 (#18281)

* grafana/master: (82 commits) TablePanel: Remove scroll option on TablePanel (grafana#18318) Keyboard Shortcuts: Sign in to enable them (grafana#18271) GitHub Templates: Pull Request Template update (grafana#18300) Auth Proxy: Include additional headers as part of the cache key (grafana#18298) grafana/toolkit: support windows paths (grafana#18306) Chore: noImplicitAny Sub 500 errors (grafana#18287) Plugins: return a promise for loadPluginCss (grafana#18273) Utils: avoid calling console.warn() too often for deprecation warnings (grafana#18269) CLI: Allow installing custom binary plugins (grafana#17551) Docs: Update link to example app (grafana#18253) GettingStarted: Skip Query for getting started (grafana#18268) v6.3.0-beta2 is latest testing (grafana#18283) Release: Changelog update with v6.3.0-beta2 (grafana#18281) Chore: Upgrades typescript to version 3.5 (grafana#18263) docs: team sync (grafana#18239) SAML: Only show SAML login button on Enterprise version (grafana#18270) Permissions: Show plugins in nav for non admin users but hide plugin configuration (grafana#18234) CI: Change target branch in CI task trigger-docs-update (grafana#18255) Plugins: Include build number and PR in metadata (grafana#18260) Run End-to-End tests for release builds (grafana#18211) ...

Allow installing custom binary plugins

383fe5f

aocenas commented Jun 12, 2019

View reviewed changes

pkg/cmd/grafana-cli/commands/install_command.go Show resolved Hide resolved

aocenas commented Jun 12, 2019

View reviewed changes

aocenas requested review from bergquist, briangann and DanCech June 12, 2019 15:28

aocenas added 5 commits June 13, 2019 20:40

Check extracted file paths and allow symlinks only for internal plugins

515e62e

Take platform into account when printing and upgrading plugins

5887d2b

Checksum field rename

a95e751

Fix naming after response changes and safe path check

17bb71a

Refactor API calls and add tests

5227440

aocenas commented Jun 18, 2019

View reviewed changes

Fix revive check

ea37715

aocenas mentioned this pull request Jun 18, 2019

Package plugin via pkg PoC grafana/grafana-image-renderer#19

Merged

3 tasks

aocenas marked this pull request as ready for review June 18, 2019 12:56

aocenas changed the title ~~Plugins: Allow installing custom binary plugins~~ CLI: Allow installing custom binary plugins Jun 18, 2019

aocenas added area/backend area/grafana-cli area/grafana.com labels Jun 18, 2019

aocenas requested a review from marefr June 19, 2019 14:37

bergquist reviewed Jun 19, 2019

View reviewed changes

gotjosh self-requested a review June 20, 2019 08:06

aocenas added 6 commits June 21, 2019 10:59

Merge branch 'master' into binary-plugins-install

57fcb9f

Merge branch 'master' into binary-plugins-install

296e9ca

Add better error handling for install requests

c00a4c6

Update comment

a6992eb

Add tests for isPathSafe

37f249c

Fix test

8bef14b

aocenas added 2 commits July 10, 2019 15:37

Fix lint

4535f66

Fix lint

312e025

marefr requested changes Jul 11, 2019

View reviewed changes

marefr added this to the 6.4 milestone Jul 11, 2019

aocenas added 5 commits July 11, 2019 09:23

Merge branch 'master' into binary-plugins-install

24b4986

Fix comments

585501f

Remove convey

f9445d7

Logging and errors fixes

06f8719

Refactor isDev check

ace2332

marefr approved these changes Jul 11, 2019

View reviewed changes

marefr added the pr/needs-manual-testing Before merge some help with manual testing & verification is requested label Jul 11, 2019

marefr mentioned this pull request Jul 12, 2019

Remove/deprecate PhantomJS #18083

Closed

7 tasks

aocenas added 2 commits July 19, 2019 10:32

Merge branch 'master' into binary-plugins-install

003d6d0

Remove timeout from download request

9ca9310

aocenas merged commit 8c49d27 into master Jul 29, 2019

aocenas deleted the binary-plugins-install branch July 29, 2019 08:45

jschill mentioned this pull request Aug 7, 2019

Remove PhantomJS #13802

Closed

5 tasks

marefr mentioned this pull request Sep 5, 2019

Deprecate PhantomJS #18914

Closed

6 tasks

aocenas added the add to changelog label Sep 17, 2019

olivierlemasle mentioned this pull request Sep 29, 2019

grafana-cli: cannot install anymore plugin with specific version #19489

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CLI: Allow installing custom binary plugins #17551

CLI: Allow installing custom binary plugins #17551

aocenas commented Jun 12, 2019 •

edited by marefr

Loading

aocenas Jun 12, 2019

aocenas Jun 12, 2019

marefr Jun 24, 2019 •

edited

Loading

DanCech Jul 9, 2019

aocenas Jun 12, 2019

aocenas Jun 18, 2019

aocenas Jun 19, 2019

torkelo Jun 19, 2019

aocenas Jun 19, 2019

aocenas Jun 19, 2019

bergquist Jun 19, 2019

aocenas Jun 20, 2019

torkelo Jun 20, 2019

aocenas Jun 20, 2019 •

edited

Loading

torkelo Jun 20, 2019

bergquist Jun 19, 2019

aocenas Jul 10, 2019

ryantxu Jul 17, 2019

aocenas Jul 17, 2019

bergquist Jun 19, 2019

aocenas Jul 10, 2019

bergquist Jun 19, 2019

bergquist Jun 19, 2019

bergquist Jun 19, 2019

gotjosh Jun 20, 2019

aocenas Jun 20, 2019

marefr left a comment •

edited

Loading

marefr left a comment

CLI: Allow installing custom binary plugins #17551

CLI: Allow installing custom binary plugins #17551

Conversation

aocenas commented Jun 12, 2019 • edited by marefr Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marefr Jun 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aocenas Jun 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marefr left a comment • edited Loading

Choose a reason for hiding this comment

marefr left a comment

Choose a reason for hiding this comment

aocenas commented Jun 12, 2019 •

edited by marefr

Loading

marefr Jun 24, 2019 •

edited

Loading

aocenas Jun 20, 2019 •

edited

Loading

marefr left a comment •

edited

Loading