From cb9998cd8ff2c737ca529025ae2b772436399f55 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Mon, 19 Nov 2018 14:23:25 -0800 Subject: [PATCH] Mount tune options (#5809) * Refactor mount tune to support upsert options values and unset options. * Do not allow unsetting options map * add secret tune version regression test * Only accept valid options version * s/meVersion/optVersion/ --- command/secrets_tune_test.go | 74 ++++++++++++++++++++++++++++++++++++ http/sys_mount_test.go | 2 +- vault/logical_system.go | 69 +++++++++++++++++++++------------ 3 files changed, 120 insertions(+), 25 deletions(-) diff --git a/command/secrets_tune_test.go b/command/secrets_tune_test.go index 0f40782907df..42bd800dc1ab 100644 --- a/command/secrets_tune_test.go +++ b/command/secrets_tune_test.go @@ -70,6 +70,80 @@ func TestSecretsTuneCommand_Run(t *testing.T) { } }) + t.Run("protect_downgrade", func(t *testing.T) { + t.Parallel() + client, closer := testVaultServer(t) + defer closer() + + ui, cmd := testSecretsTuneCommand(t) + cmd.client = client + + // Mount + if err := client.Sys().Mount("kv", &api.MountInput{ + Type: "kv", + Options: map[string]string{ + "version": "2", + }, + }); err != nil { + t.Fatal(err) + } + + // confirm default max_versions + mounts, err := client.Sys().ListMounts() + if err != nil { + t.Fatal(err) + } + + mountInfo, ok := mounts["kv/"] + if !ok { + t.Fatalf("expected mount to exist") + } + if exp := "kv"; mountInfo.Type != exp { + t.Errorf("expected %q to be %q", mountInfo.Type, exp) + } + if exp := "2"; mountInfo.Options["version"] != exp { + t.Errorf("expected %q to be %q", mountInfo.Options["version"], exp) + } + + if exp := ""; mountInfo.Options["max_versions"] != exp { + t.Errorf("expected %s to be empty", mountInfo.Options["max_versions"]) + } + + // omitting the version should not cause a downgrade + code := cmd.Run([]string{ + "-options", "max_versions=2", + "kv/", + }) + if exp := 0; code != exp { + t.Errorf("expected %d to be %d", code, exp) + } + + expected := "Success! Tuned the secrets engine at: kv/" + combined := ui.OutputWriter.String() + ui.ErrorWriter.String() + if !strings.Contains(combined, expected) { + t.Errorf("expected %q to contain %q", combined, expected) + } + + mounts, err = client.Sys().ListMounts() + if err != nil { + t.Fatal(err) + } + + mountInfo, ok = mounts["kv/"] + if !ok { + t.Fatalf("expected mount to exist") + } + if exp := "2"; mountInfo.Options["version"] != exp { + t.Errorf("expected %q to be %q", mountInfo.Options["version"], exp) + } + if exp := "kv"; mountInfo.Type != exp { + t.Errorf("expected %q to be %q", mountInfo.Type, exp) + } + if exp := "2"; mountInfo.Options["max_versions"] != exp { + t.Errorf("expected %s to be %s", mountInfo.Options["max_versions"], exp) + } + }) + t.Run("integration", func(t *testing.T) { t.Run("flags_all", func(t *testing.T) { t.Parallel() diff --git a/http/sys_mount_test.go b/http/sys_mount_test.go index 5193b101d0fd..b5dcb168e288 100644 --- a/http/sys_mount_test.go +++ b/http/sys_mount_test.go @@ -676,7 +676,7 @@ func TestSysTuneMount_Options(t *testing.T) { t.Fatalf("bad:\nExpected: %#v\nActual:%#v", expected, actual) } - // Unset the mount tune value + // Check that we're not allowed to unset the options map once that's set resp = testHttpPost(t, token, addr+"/v1/sys/mounts/foo/tune", map[string]interface{}{ "options": map[string]string{}, }) diff --git a/vault/logical_system.go b/vault/logical_system.go index 1c043f6318e4..c72a8f2e7617 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -1340,14 +1340,14 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string, if optionsRaw, ok := data.GetOk("options"); ok { options = optionsRaw.(map[string]string) } + if len(options) > 0 { b.Core.logger.Info("mount tuning of options", "path", path, "options", options) + newOptions := make(map[string]string) + var kvUpgraded bool - var changed bool - var numBuiltIn int + // The version options should only apply to the KV mount, check that first if v, ok := options["version"]; ok { - changed = true - numBuiltIn++ // Special case to make sure we can not disable versioning once it's // enabled. If the vkv backend suports downgrading this can be removed. meVersion, err := parseutil.ParseInt(mountEntry.Options["version"]) @@ -1358,38 +1358,59 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string, if err != nil { return handleError(errwrap.Wrapf("unable to parse options: {{err}}", err)) } + + // Only accept valid versions + switch optVersion { + case 1: + case 2: + default: + return logical.ErrorResponse(fmt.Sprintf("invalid version provided: %d", optVersion)), logical.ErrInvalidRequest + } + if meVersion > optVersion { + // Return early if version option asks for a downgrade return logical.ErrorResponse(fmt.Sprintf("cannot downgrade mount from version %d", meVersion)), logical.ErrInvalidRequest } if meVersion < optVersion { + kvUpgraded = true resp = &logical.Response{} resp.AddWarning(fmt.Sprintf("Upgrading mount from version %d to version %d. This mount will be unavailable for a brief period and will resume service shortly.", meVersion, optVersion)) } } - if options != nil { - // For anything we don't recognize and provide special handling, - // always write - if len(options) > numBuiltIn { - changed = true + + // Upsert options value to a copy of the existing mountEntry's options + for k, v := range mountEntry.Options { + newOptions[k] = v + } + for k, v := range options { + // If the value of the provided option is empty, delete the key We + // special-case the version value here to guard against KV downgrades, but + // this piece could potentially be refactored in the future to be non-KV + // specific. + if len(v) == 0 && k != "version" { + delete(newOptions, k) + } else { + newOptions[k] = v } } - if changed { - oldVal := mountEntry.Options - mountEntry.Options = options - // Update the mount table - switch { - case strings.HasPrefix(path, "auth/"): - err = b.Core.persistAuth(ctx, b.Core.auth, &mountEntry.Local) - default: - err = b.Core.persistMounts(ctx, b.Core.mounts, &mountEntry.Local) - } - if err != nil { - mountEntry.Options = oldVal - return handleError(err) - } + // Update the mount table + oldVal := mountEntry.Options + mountEntry.Options = newOptions + switch { + case strings.HasPrefix(path, "auth/"): + err = b.Core.persistAuth(ctx, b.Core.auth, &mountEntry.Local) + default: + err = b.Core.persistMounts(ctx, b.Core.mounts, &mountEntry.Local) + } + if err != nil { + mountEntry.Options = oldVal + return handleError(err) + } - // Reload the backend to kick off the upgrade process. + // Reload the backend to kick off the upgrade process. It should only apply to KV backend so we + // trigger based on the version logic above. + if kvUpgraded { b.Core.reloadBackendCommon(ctx, mountEntry, strings.HasPrefix(path, credentialRoutePrefix)) } }