From facd4eca7eddc4fdd3960dc841b62f6db624a49a Mon Sep 17 00:00:00 2001 From: Giuseppe Guerra Date: Tue, 11 Apr 2023 17:15:34 +0200 Subject: [PATCH] Plugins: Fix plugin signature calculation not working on Windows (#66273) * Plugins: Fix plugin signature calculation not working on Windows * Plugins: Added test for NTFS path separators in signature verification * Use filepath.ToSlash and replace its implementation in tests * Fix typo --- pkg/plugins/manager/signature/manifest.go | 11 +- .../manager/signature/manifest_test.go | 130 ++++++++++++++++++ 2 files changed, 140 insertions(+), 1 deletion(-) diff --git a/pkg/plugins/manager/signature/manifest.go b/pkg/plugins/manager/signature/manifest.go index 1bd03488aae..4f7923b067e 100644 --- a/pkg/plugins/manager/signature/manifest.go +++ b/pkg/plugins/manager/signature/manifest.go @@ -12,6 +12,7 @@ import ( "net/url" "os" "path" + "path/filepath" "runtime" "strings" @@ -55,7 +56,12 @@ N1c5v9v/4h6qeA== -----END PGP PUBLIC KEY BLOCK----- ` -var runningWindows = runtime.GOOS == "windows" +var ( + runningWindows = runtime.GOOS == "windows" + + // toSlash is filepath.ToSlash, but can be overwritten in tests path separators cross-platform + toSlash = filepath.ToSlash +) // PluginManifest holds details for the file manifest type PluginManifest struct { @@ -194,6 +200,9 @@ func Calculate(ctx context.Context, mlog log.Logger, src plugins.PluginSource, p // Track files missing from the manifest var unsignedFiles []string for _, f := range plugin.FS.Files() { + // Ensure slashes are used, because MANIFEST.txt always uses slashes regardless of the filesystem + f = toSlash(f) + // Ignoring unsigned Chromium debug.log so it doesn't invalidate the signature for Renderer plugin running on Windows if runningWindows && plugin.JSONData.Type == plugins.Renderer && f == "chrome-win/debug.log" { continue diff --git a/pkg/plugins/manager/signature/manifest_test.go b/pkg/plugins/manager/signature/manifest_test.go index 6683523762a..e3157c1b239 100644 --- a/pkg/plugins/manager/signature/manifest_test.go +++ b/pkg/plugins/manager/signature/manifest_test.go @@ -210,6 +210,136 @@ func TestCalculate(t *testing.T) { SigningOrg: "Grafana Labs", }, sig) }) + + t.Run("Signature verification should work with any path separator", func(t *testing.T) { + var toSlashUnix = newToSlash('/') + var toSlashWindows = newToSlash('\\') + + for _, tc := range []struct { + name string + sep string + toSlash func(string) string + }{ + {"unix", "/", toSlashUnix}, + {"windows", "\\", toSlashWindows}, + } { + t.Run(tc.name, func(t *testing.T) { + // Replace toSlash for cross-platform testing + oldToSlash := toSlash + t.Cleanup(func() { + toSlash = oldToSlash + }) + toSlash = tc.toSlash + + basePath := "../testdata/app-with-child/dist" + + sig, err := Calculate(context.Background(), log.NewTestLogger(), &fakes.FakePluginSource{ + PluginClassFunc: func(ctx context.Context) plugins.Class { + return plugins.External + }, + }, plugins.FoundPlugin{ + JSONData: plugins.JSONData{ + ID: "myorgid-simple-app", + Type: plugins.App, + Info: plugins.Info{ + Version: "%VERSION%", + }, + }, + FS: newPathSeparatorOverrideFS(tc.sep, map[string]struct{}{ + filepath.Join(basePath, "MANIFEST.txt"): {}, + filepath.Join(basePath, "plugin.json"): {}, + filepath.Join(basePath, "child/plugin.json"): {}, + }, basePath), + }) + require.NoError(t, err) + require.Equal(t, plugins.Signature{ + Status: plugins.SignatureValid, + Type: plugins.GrafanaSignature, + SigningOrg: "Grafana Labs", + }, sig) + }) + } + }) +} + +// newToSlash returns a new function that acts as filepath.ToSlash but for the specified os-separator. +// This can be used to test filepath.ToSlash-dependant code cross-platform. +func newToSlash(sep rune) func(string) string { + return func(path string) string { + if sep == '/' { + return path + } + return strings.ReplaceAll(path, string(sep), "/") + } +} + +func TestNewToSlash(t *testing.T) { + t.Run("unix", func(t *testing.T) { + toSlashUnix := newToSlash('/') + require.Equal(t, "folder", toSlashUnix("folder")) + require.Equal(t, "/folder", toSlashUnix("/folder")) + require.Equal(t, "/folder/file", toSlashUnix("/folder/file")) + require.Equal(t, "/folder/other\\file", toSlashUnix("/folder/other\\file")) + }) + + t.Run("windows", func(t *testing.T) { + toSlashWindows := newToSlash('\\') + require.Equal(t, "folder", toSlashWindows("folder")) + require.Equal(t, "C:/folder", toSlashWindows("C:\\folder")) + require.Equal(t, "folder/file.exe", toSlashWindows("folder\\file.exe")) + }) +} + +// fsPathSeparatorFiles embeds plugins.LocalFS and overrides the Files() behaviour so all the returned elements +// have the specified path separator. This can be used to test Files() behaviour cross-platform. +type fsPathSeparatorFiles struct { + plugins.LocalFS + + separator string +} + +// newPathSeparatorOverrideFS returns a new fsPathSeparatorFiles. Sep is the separator that will be used ONLY for +// the elements returned by Files(). Files and basePath MUST use the os-specific path separator (filepath.Separator) +// if Open() is required to work for the test case. +func newPathSeparatorOverrideFS(sep string, files map[string]struct{}, basePath string) fsPathSeparatorFiles { + return fsPathSeparatorFiles{ + LocalFS: plugins.NewLocalFS(files, basePath), + separator: sep, + } +} + +// Files returns LocalFS.Files(), but all path separators (filepath.Separator) are replaced with f.separator. +func (f fsPathSeparatorFiles) Files() []string { + files := f.LocalFS.Files() + const osSepStr = string(filepath.Separator) + for i := 0; i < len(files); i++ { + files[i] = strings.ReplaceAll(files[i], osSepStr, f.separator) + } + return files +} + +func TestFSPathSeparatorFiles(t *testing.T) { + for _, tc := range []struct { + name string + sep string + }{ + {"unix", "/"}, + {"windows", "\\"}, + } { + t.Run(tc.name, func(t *testing.T) { + fs := newPathSeparatorOverrideFS("/", map[string]struct{}{ + "a": {}, + strings.Join([]string{"a", "b", "c"}, tc.sep): {}, + }, ".") + files := fs.Files() + filesMap := make(map[string]struct{}, len(files)) + // Re-convert to map as the key order is not stable + for _, f := range files { + filesMap[f] = struct{}{} + } + require.Equal(t, filesMap, map[string]struct{}{"a": {}, strings.Join([]string{"a", "b", "c"}, tc.sep): {}}) + }) + } } func fileList(manifest *PluginManifest) []string {