From 4bd5af87f4368c0f2669d782ef805db12d90629d Mon Sep 17 00:00:00 2001 From: Tom Proctor Date: Fri, 30 Sep 2022 10:33:31 +0100 Subject: [PATCH] Plugins: Fix file permissions check to always use the correct path (#17340) * Add failing test for when command != plugin name * wrapFactoryCheckPerms uses pluginCatalog.Get to fetch the correct command * Use filepath.Rel for consistency with plugin read API handler --- changelog/17340.txt | 3 ++ vault/external_plugin_test.go | 81 +++++++++++++++++++++++++++++++++-- vault/plugin_catalog.go | 28 +++++++++++- 3 files changed, 107 insertions(+), 5 deletions(-) create mode 100644 changelog/17340.txt diff --git a/changelog/17340.txt b/changelog/17340.txt new file mode 100644 index 000000000..733c737a0 --- /dev/null +++ b/changelog/17340.txt @@ -0,0 +1,3 @@ +```release-note:bug +plugins: Corrected the path to check permissions on when the registered plugin name does not match the plugin binary's filename. +``` diff --git a/vault/external_plugin_test.go b/vault/external_plugin_test.go index b0b570d6f..df1bab816 100644 --- a/vault/external_plugin_test.go +++ b/vault/external_plugin_test.go @@ -509,11 +509,84 @@ func TestExternalPlugin_getBackendTypeVersion(t *testing.T) { } } +func TestExternalPlugin_CheckFilePermissions(t *testing.T) { + // Turn on the check. + if err := os.Setenv(consts.VaultEnableFilePermissionsCheckEnv, "true"); err != nil { + t.Fatal(err) + } + defer func() { + if err := os.Unsetenv(consts.VaultEnableFilePermissionsCheckEnv); err != nil { + t.Fatal(err) + } + }() + + for name, tc := range map[string]struct { + pluginNameFmt string + pluginType consts.PluginType + pluginVersion string + }{ + "plugin name and file name match": { + pluginNameFmt: "%s", + pluginType: consts.PluginTypeCredential, + }, + "plugin name and file name mismatch": { + pluginNameFmt: "%s-foo", + pluginType: consts.PluginTypeSecrets, + }, + "plugin name has slash": { + pluginNameFmt: "%s/foo", + pluginType: consts.PluginTypeCredential, + }, + "plugin with version": { + pluginNameFmt: "%s/foo", + pluginType: consts.PluginTypeCredential, + pluginVersion: "v1.2.3", + }, + } { + t.Run(name, func(t *testing.T) { + c, pluginName, pluginSHA256 := testCoreWithPlugin(t, tc.pluginType, tc.pluginVersion) + registeredPluginName := fmt.Sprintf(tc.pluginNameFmt, pluginName) + + // Permissions will be checked once during registration. + req := logical.TestRequest(t, logical.UpdateOperation, fmt.Sprintf("plugins/catalog/%s/%s", tc.pluginType.String(), registeredPluginName)) + req.Data = map[string]interface{}{ + "command": pluginName, + "sha256": pluginSHA256, + "version": tc.pluginVersion, + } + resp, err := c.systemBackend.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + t.Fatal(err) + } + if resp.Error() != nil { + t.Fatal(resp.Error()) + } + + // Now attempt to mount the plugin, which should trigger checking the permissions again. + req = logical.TestRequest(t, logical.UpdateOperation, mountTable(tc.pluginType)) + req.Data = map[string]interface{}{ + "type": registeredPluginName, + } + if tc.pluginVersion != "" { + req.Data["config"] = map[string]interface{}{ + "plugin_version": tc.pluginVersion, + } + } + resp, err = c.systemBackend.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + t.Fatal(err) + } + if resp.Error() != nil { + t.Fatal(resp.Error()) + } + }) + } +} + func registerPlugin(t *testing.T, sys *SystemBackend, pluginName, pluginType, version, sha string) { t.Helper() req := logical.TestRequest(t, logical.UpdateOperation, fmt.Sprintf("plugins/catalog/%s/%s", pluginType, pluginName)) req.Data = map[string]interface{}{ - "name": pluginName, "command": pluginName, "sha256": sha, "version": version, @@ -523,7 +596,7 @@ func registerPlugin(t *testing.T, sys *SystemBackend, pluginName, pluginType, ve t.Fatal(err) } if resp.Error() != nil { - t.Fatalf("%#v", resp) + t.Fatal(resp.Error()) } } @@ -543,7 +616,7 @@ func mountPlugin(t *testing.T, sys *SystemBackend, pluginName string, pluginType t.Fatal(err) } if resp.Error() != nil { - t.Fatalf("%#v", resp) + t.Fatal(resp.Error()) } } @@ -554,6 +627,6 @@ func mountTable(pluginType consts.PluginType) string { case consts.PluginTypeSecrets: return "mounts/foo" default: - panic("test does not support plugin type yet") + panic("test does not support mounting plugin type yet: " + pluginType.String()) } } diff --git a/vault/plugin_catalog.go b/vault/plugin_catalog.go index 8fb886057..1ffd347c2 100644 --- a/vault/plugin_catalog.go +++ b/vault/plugin_catalog.go @@ -89,7 +89,33 @@ type pluginClient struct { func wrapFactoryCheckPerms(core *Core, f logical.Factory) logical.Factory { return func(ctx context.Context, conf *logical.BackendConfig) (logical.Backend, error) { - if err := core.CheckPluginPerms(conf.Config["plugin_name"]); err != nil { + pluginName := conf.Config["plugin_name"] + pluginVersion := conf.Config["plugin_version"] + pluginTypeRaw := conf.Config["plugin_type"] + pluginType, err := consts.ParsePluginType(pluginTypeRaw) + if err != nil { + return nil, err + } + + pluginDescription := fmt.Sprintf("%s plugin %s", pluginTypeRaw, pluginName) + if pluginVersion != "" { + pluginDescription += " version " + pluginVersion + } + + plugin, err := core.pluginCatalog.Get(ctx, pluginName, pluginType, pluginVersion) + if err != nil { + return nil, fmt.Errorf("failed to find %s in plugin catalog: %w", pluginDescription, err) + } + if plugin == nil { + return nil, fmt.Errorf("failed to find %s in plugin catalog", pluginDescription) + } + + command, err := filepath.Rel(core.pluginCatalog.directory, plugin.Command) + if err != nil { + return nil, fmt.Errorf("failed to compute plugin command: %w", err) + } + + if err := core.CheckPluginPerms(command); err != nil { return nil, err } return f(ctx, conf)