Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mount tune options #5809

Merged
merged 7 commits into from
Nov 19, 2018
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 74 additions & 0 deletions command/secrets_tune_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
2 changes: 1 addition & 1 deletion http/sys_mount_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{},
})
Expand Down
60 changes: 36 additions & 24 deletions vault/logical_system.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"])
Expand All @@ -1359,37 +1359,49 @@ func (b *SystemBackend) handleTuneWriteCommon(ctx context.Context, path string,
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 {
calvn marked this conversation as resolved.
Show resolved Hide resolved
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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@briankassouf can you confirm that the comment/logic is true? If false, I can remove the kvUpgraded check and always run reloadBackendCommon whenever options gets updated.

b.Core.reloadBackendCommon(ctx, mountEntry, strings.HasPrefix(path, credentialRoutePrefix))
}
}
Expand Down