From 266d883f5bc57e828f3122d73e6c57c9e22a0e1b Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Wed, 9 Nov 2022 09:55:18 -0500 Subject: [PATCH 1/5] plugins: Filter builtins by RunningVersion This commit adds some logic to handle the case where a mount entry has a non-builtin RunningVersion. This ensures that we only report deprecation status for builtins. This resolves VAULT-9019. --- vault/logical_system.go | 2 +- vault/mount.go | 11 ++++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index d67ce5f2fc69d..303bc8908f22b 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -956,7 +956,7 @@ func (b *SystemBackend) mountInfo(ctx context.Context, entry *MountEntry) map[st } // Add deprecation status only if it exists - builtinType := b.Core.builtinTypeFromMountEntry(ctx, entry) + builtinType := b.Core.builtinTypeFromMountEntry(ctx, entry, entry.RunningVersion) if status, ok := b.Core.builtinRegistry.DeprecationStatus(entry.Type, builtinType); ok { info["deprecation_status"] = status.String() } diff --git a/vault/mount.go b/vault/mount.go index b3b76396ab41f..574bbc4d116f3 100644 --- a/vault/mount.go +++ b/vault/mount.go @@ -712,13 +712,18 @@ func (c *Core) mountInternal(ctx context.Context, entry *MountEntry, updateStora // builtinTypeFromMountEntry attempts to find a builtin PluginType associated // with the specified MountEntry. Returns consts.PluginTypeUnknown if not found. -func (c *Core) builtinTypeFromMountEntry(ctx context.Context, entry *MountEntry) consts.PluginType { +func (c *Core) builtinTypeFromMountEntry(ctx context.Context, entry *MountEntry, version string) consts.PluginType { if c.builtinRegistry == nil || entry == nil { return consts.PluginTypeUnknown } + // Builtin plugins should contain the "builtin" string in their RunningVersion + if !strings.Contains(version, "builtin") { + return consts.PluginTypeUnknown + } + builtinPluginType := func(name string, pluginType consts.PluginType) (consts.PluginType, bool) { - plugin, err := c.pluginCatalog.Get(ctx, name, pluginType, "") + plugin, err := c.pluginCatalog.Get(ctx, name, pluginType, version) if err == nil && plugin != nil && plugin.Builtin { return plugin.Type, true } @@ -964,7 +969,7 @@ func (c *Core) handleDeprecatedMountEntry(ctx context.Context, entry *MountEntry // Allow type to be determined from mount entry when not otherwise specified if pluginType == consts.PluginTypeUnknown { - pluginType = c.builtinTypeFromMountEntry(ctx, entry) + pluginType = c.builtinTypeFromMountEntry(ctx, entry, entry.RunningVersion) } // Handle aliases From 1852a34654ac007c475d69e5c370a05e7ddf9407 Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Wed, 9 Nov 2022 10:13:08 -0500 Subject: [PATCH 2/5] Changelog for vault-9019 --- changelog/17816.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/17816.txt diff --git a/changelog/17816.txt b/changelog/17816.txt new file mode 100644 index 0000000000000..4ad3bc8622c40 --- /dev/null +++ b/changelog/17816.txt @@ -0,0 +1,3 @@ +```release-note:bug +plugins: Only report deprecation status for builtin plugins. +``` From 0a10e946ea421673b60f61d50021a8347b232283 Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Fri, 11 Nov 2022 08:51:40 -0500 Subject: [PATCH 3/5] Drop unnecessary version argument --- vault/logical_system.go | 2 +- vault/mount.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index 303bc8908f22b..d67ce5f2fc69d 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -956,7 +956,7 @@ func (b *SystemBackend) mountInfo(ctx context.Context, entry *MountEntry) map[st } // Add deprecation status only if it exists - builtinType := b.Core.builtinTypeFromMountEntry(ctx, entry, entry.RunningVersion) + builtinType := b.Core.builtinTypeFromMountEntry(ctx, entry) if status, ok := b.Core.builtinRegistry.DeprecationStatus(entry.Type, builtinType); ok { info["deprecation_status"] = status.String() } diff --git a/vault/mount.go b/vault/mount.go index 574bbc4d116f3..a2b5bc5219a13 100644 --- a/vault/mount.go +++ b/vault/mount.go @@ -712,18 +712,18 @@ func (c *Core) mountInternal(ctx context.Context, entry *MountEntry, updateStora // builtinTypeFromMountEntry attempts to find a builtin PluginType associated // with the specified MountEntry. Returns consts.PluginTypeUnknown if not found. -func (c *Core) builtinTypeFromMountEntry(ctx context.Context, entry *MountEntry, version string) consts.PluginType { +func (c *Core) builtinTypeFromMountEntry(ctx context.Context, entry *MountEntry) consts.PluginType { if c.builtinRegistry == nil || entry == nil { return consts.PluginTypeUnknown } // Builtin plugins should contain the "builtin" string in their RunningVersion - if !strings.Contains(version, "builtin") { + if !strings.Contains(entry.RunningVersion, "builtin") { return consts.PluginTypeUnknown } builtinPluginType := func(name string, pluginType consts.PluginType) (consts.PluginType, bool) { - plugin, err := c.pluginCatalog.Get(ctx, name, pluginType, version) + plugin, err := c.pluginCatalog.Get(ctx, name, pluginType, entry.RunningVersion) if err == nil && plugin != nil && plugin.Builtin { return plugin.Type, true } @@ -969,7 +969,7 @@ func (c *Core) handleDeprecatedMountEntry(ctx context.Context, entry *MountEntry // Allow type to be determined from mount entry when not otherwise specified if pluginType == consts.PluginTypeUnknown { - pluginType = c.builtinTypeFromMountEntry(ctx, entry, entry.RunningVersion) + pluginType = c.builtinTypeFromMountEntry(ctx, entry) } // Handle aliases From f6e964c928b419896bce86ef95347f89cb930ea5 Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Fri, 11 Nov 2022 12:49:38 -0500 Subject: [PATCH 4/5] Add test for auth/plugin decoupling --- vault/external_plugin_test.go | 116 ++++++++++++++++++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/vault/external_plugin_test.go b/vault/external_plugin_test.go index 0dd4ec5ed2f38..969fafa76089c 100644 --- a/vault/external_plugin_test.go +++ b/vault/external_plugin_test.go @@ -277,6 +277,85 @@ func TestCore_EnableExternalPlugin_MultipleVersions(t *testing.T) { } } +func TestCore_EnableExternalPlugin_ShadowBuiltin(t *testing.T) { + pluginDir, cleanup := MakeTestPluginDir(t) + t.Cleanup(func() { cleanup(t) }) + + // new kv plugin can be registered but not mounted + plugin := compilePlugin(t, consts.PluginTypeCredential, "v1.2.3", pluginDir) + err := os.Link(path.Join(pluginDir, plugin.fileName), path.Join(pluginDir, "app-id")) + if err != nil { + t.Fatal(err) + } + pluginName := "approle" + conf := &CoreConfig{ + BuiltinRegistry: NewMockBuiltinRegistry(), + PluginDirectory: pluginDir, + } + c := TestCoreWithSealAndUI(t, conf) + c, _, _ = testCoreUnsealed(t, c) + + verifyAuthListDeprecationStatus := func(authName string, checkExists bool) error { + req := logical.TestRequest(t, logical.ReadOperation, mountTable(consts.PluginTypeCredential)) + req.Data = map[string]interface{}{ + "type": authName, + } + resp, err := c.systemBackend.HandleRequest(namespace.RootContext(nil), req) + if err != nil { + return err + } + status := resp.Data["deprecation_status"] + if checkExists && status == nil { + return fmt.Errorf("expected deprecation status but found none") + } else if !checkExists && status != nil { + return fmt.Errorf("expected nil deprecation status but found %q", status) + } + return nil + } + + // Create a new auth method with builtin approle + mountPlugin(t, c.systemBackend, pluginName, consts.PluginTypeCredential, "", "") + + // Read the auth table to verify deprecation status + if err := verifyAuthListDeprecationStatus(pluginName, true); err != nil { + t.Fatal(err) + } + + // Register a shadow plugin + registerPlugin(t, c.systemBackend, pluginName, consts.PluginTypeCredential.String(), "v1.2.3", plugin.sha256, plugin.fileName) + + // Verify auth table hasn't changed + if err := verifyAuthListDeprecationStatus(pluginName, true); err != nil { + t.Fatal(err) + } + + // Remount auth method using registered shadow plugin + unmountPlugin(t, c.systemBackend, pluginName, consts.PluginTypeCredential, "", "") + mountPlugin(t, c.systemBackend, pluginName, consts.PluginTypeCredential, "", "") + + // Verify auth table has changed + if err := verifyAuthListDeprecationStatus(pluginName, false); err != nil { + t.Fatal(err) + } + + // Deregister shadow plugin + deregisterPlugin(t, c.systemBackend, pluginName, consts.PluginTypeSecrets.String(), "v1.2.3", plugin.sha256, plugin.fileName) + + // Verify auth table hasn't changed + if err := verifyAuthListDeprecationStatus(pluginName, false); err != nil { + t.Fatal(err) + } + + // Remount auth method + unmountPlugin(t, c.systemBackend, pluginName, consts.PluginTypeCredential, "", "") + mountPlugin(t, c.systemBackend, pluginName, consts.PluginTypeCredential, "", "") + + // Verify auth table has changed + if err := verifyAuthListDeprecationStatus(pluginName, false); err != nil { + t.Fatal(err) + } +} + func TestCore_EnableExternalKv_MultipleVersions(t *testing.T) { pluginDir, cleanup := MakeTestPluginDir(t) t.Cleanup(func() { cleanup(t) }) @@ -759,6 +838,43 @@ func mountPlugin(t *testing.T, sys *SystemBackend, pluginName string, pluginType } } +func unmountPlugin(t *testing.T, sys *SystemBackend, pluginName string, pluginType consts.PluginType, version, path string) { + t.Helper() + var mountPath string + if path == "" { + mountPath = mountTable(pluginType) + } else { + mountPath = mountTableWithPath(consts.PluginTypeSecrets, path) + } + req := logical.TestRequest(t, logical.DeleteOperation, mountPath) + req.Data = map[string]interface{}{ + "type": pluginName, + } + if version != "" { + req.Data["config"] = map[string]interface{}{ + "plugin_version": version, + } + } + resp, err := sys.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } +} + +func deregisterPlugin(t *testing.T, sys *SystemBackend, pluginName, pluginType, version, sha, command string) { + t.Helper() + req := logical.TestRequest(t, logical.DeleteOperation, fmt.Sprintf("plugins/catalog/%s/%s", pluginType, pluginName)) + req.Data = map[string]interface{}{ + "command": command, + "sha256": sha, + "version": version, + } + resp, err := sys.HandleRequest(namespace.RootContext(nil), req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } +} + func mountTable(pluginType consts.PluginType) string { return mountTableWithPath(pluginType, "foo") } From 6a01a3957124138e7ed247b8743dfd5eff5d11ba Mon Sep 17 00:00:00 2001 From: Mike Palmiotto Date: Fri, 11 Nov 2022 13:02:29 -0500 Subject: [PATCH 5/5] Couple of cleanups --- vault/external_plugin_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/vault/external_plugin_test.go b/vault/external_plugin_test.go index 969fafa76089c..cb4761091a287 100644 --- a/vault/external_plugin_test.go +++ b/vault/external_plugin_test.go @@ -281,9 +281,9 @@ func TestCore_EnableExternalPlugin_ShadowBuiltin(t *testing.T) { pluginDir, cleanup := MakeTestPluginDir(t) t.Cleanup(func() { cleanup(t) }) - // new kv plugin can be registered but not mounted + // create an external plugin to shadow the builtin "approle" plugin := compilePlugin(t, consts.PluginTypeCredential, "v1.2.3", pluginDir) - err := os.Link(path.Join(pluginDir, plugin.fileName), path.Join(pluginDir, "app-id")) + err := os.Link(path.Join(pluginDir, plugin.fileName), path.Join(pluginDir, "approle")) if err != nil { t.Fatal(err) }