From 76a9ce2319d65414a92ac027072d4177cd5b67a3 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Wed, 14 Nov 2018 00:41:52 -0800 Subject: [PATCH 1/5] Refactor mount tune to support upsert options values and unset options. --- http/sys_mount_test.go | 2 - vault/logical_system.go | 96 ++++++++++++++++++++++------------------- 2 files changed, 52 insertions(+), 46 deletions(-) diff --git a/http/sys_mount_test.go b/http/sys_mount_test.go index 5193b101d0fd..95eb61e5deec 100644 --- a/http/sys_mount_test.go +++ b/http/sys_mount_test.go @@ -698,12 +698,10 @@ func TestSysTuneMount_Options(t *testing.T) { "default_lease_ttl": json.Number("2764800"), "max_lease_ttl": json.Number("2764800"), "force_no_cache": false, - "options": map[string]interface{}{"test": "true"}, }, "default_lease_ttl": json.Number("2764800"), "max_lease_ttl": json.Number("2764800"), "force_no_cache": false, - "options": map[string]interface{}{"test": "true"}, } testResponseBody(t, resp, &actual) expected["request_id"] = actual["request_id"] diff --git a/vault/logical_system.go b/vault/logical_system.go index 2ea39bfd8018..92fe90ff6ecb 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -1315,58 +1315,66 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string, var resp *logical.Response var options map[string]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) - var changed bool - var numBuiltIn int - 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"]) - if err != nil { - return nil, errwrap.Wrapf("unable to parse mount entry: {{err}}", err) - } - optVersion, err := parseutil.ParseInt(v) - if err != nil { - return handleError(errwrap.Wrapf("unable to parse options: {{err}}", err)) - } - if meVersion > optVersion { - return logical.ErrorResponse(fmt.Sprintf("cannot downgrade mount from version %d", meVersion)), logical.ErrInvalidRequest + options = optionsRaw.(map[string]string) + newOptions := make(map[string]string) + var kvUpgraded bool + + if len(options) > 0 { + // The version options should only apply to the KV mount, check that first + if v, ok := options["version"]; ok { + // 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"]) + if err != nil { + return nil, errwrap.Wrapf("unable to parse mount entry: {{err}}", err) + } + optVersion, err := parseutil.ParseInt(v) + if err != nil { + return handleError(errwrap.Wrapf("unable to parse options: {{err}}", err)) + } + 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 meVersion < optVersion { - 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)) + + // Upsert options value to a copy of the existing mountEntry's options + for k, v := range mountEntry.Options { + newOptions[k] = v } - } - if options != nil { - // For anything we don't recognize and provide special handling, - // always write - if len(options) > numBuiltIn { - changed = true + for k, v := range options { + // If the value of the provided option is empty, delete the key + if len(v) == 0 { + 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 + if kvUpgraded { b.Core.reloadBackendCommon(ctx, mountEntry, strings.HasPrefix(path, credentialRoutePrefix)) } } From e3369555693cc6c248ebba1e675e81e1336184bb Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 16 Nov 2018 14:30:32 -0800 Subject: [PATCH 2/5] Do not allow unsetting options map --- http/sys_mount_test.go | 4 ++- vault/logical_system.go | 74 ++++++++++++++++++++++------------------- 2 files changed, 42 insertions(+), 36 deletions(-) diff --git a/http/sys_mount_test.go b/http/sys_mount_test.go index 95eb61e5deec..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{}, }) @@ -698,10 +698,12 @@ func TestSysTuneMount_Options(t *testing.T) { "default_lease_ttl": json.Number("2764800"), "max_lease_ttl": json.Number("2764800"), "force_no_cache": false, + "options": map[string]interface{}{"test": "true"}, }, "default_lease_ttl": json.Number("2764800"), "max_lease_ttl": json.Number("2764800"), "force_no_cache": false, + "options": map[string]interface{}{"test": "true"}, } testResponseBody(t, resp, &actual) expected["request_id"] = actual["request_id"] diff --git a/vault/logical_system.go b/vault/logical_system.go index c135b6d16e45..d3120f2cc086 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -1338,48 +1338,51 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string, var resp *logical.Response var options map[string]string if optionsRaw, ok := data.GetOk("options"); ok { - b.Core.logger.Info("mount tuning of options", "path", path, "options", options) - 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 - if len(options) > 0 { - // The version options should only apply to the KV mount, check that first - if v, ok := options["version"]; ok { - // 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"]) - if err != nil { - return nil, errwrap.Wrapf("unable to parse mount entry: {{err}}", err) - } - optVersion, err := parseutil.ParseInt(v) - if err != nil { - return handleError(errwrap.Wrapf("unable to parse options: {{err}}", err)) - } - 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)) - } + // The version options should only apply to the KV mount, check that first + if v, ok := options["version"]; ok { + // 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"]) + if err != nil { + return nil, errwrap.Wrapf("unable to parse mount entry: {{err}}", err) } + optVersion, err := parseutil.ParseInt(v) + if err != nil { + return handleError(errwrap.Wrapf("unable to parse options: {{err}}", err)) + } + 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)) + } + } - // Upsert options value to a copy of the existing mountEntry's options - for k, v := range mountEntry.Options { + // 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 } - for k, v := range options { - // If the value of the provided option is empty, delete the key - if len(v) == 0 { - delete(newOptions, k) - } else { - newOptions[k] = v - } - } } // Update the mount table @@ -1396,7 +1399,8 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string, 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)) } From 606fc76229e070c8d8f7b429d462719e505aeecc Mon Sep 17 00:00:00 2001 From: Clint Shryock Date: Wed, 14 Nov 2018 12:53:31 -0600 Subject: [PATCH 3/5] add secret tune version regression test --- command/secrets_tune_test.go | 74 ++++++++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) 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() From 99bae45a48e1f0241fee5b62799440db14c82039 Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 16 Nov 2018 16:36:22 -0800 Subject: [PATCH 4/5] Only accept valid options version --- vault/logical_system.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/vault/logical_system.go b/vault/logical_system.go index d3120f2cc086..0809d7454c3c 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -1358,6 +1358,15 @@ 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", meVersion)), 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 From 3867c5e4161d7422f149f0ca50b3e22fb1c4afdd Mon Sep 17 00:00:00 2001 From: Calvin Leung Huang Date: Fri, 16 Nov 2018 16:37:19 -0800 Subject: [PATCH 5/5] s/meVersion/optVersion/ --- vault/logical_system.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/logical_system.go b/vault/logical_system.go index 0809d7454c3c..c72a8f2e7617 100644 --- a/vault/logical_system.go +++ b/vault/logical_system.go @@ -1364,7 +1364,7 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string, case 1: case 2: default: - return logical.ErrorResponse(fmt.Sprintf("invalid version provided: %d", meVersion)), logical.ErrInvalidRequest + return logical.ErrorResponse(fmt.Sprintf("invalid version provided: %d", optVersion)), logical.ErrInvalidRequest } if meVersion > optVersion {