From 6ec33676489c8007081db8285b65fe5cd7547649 Mon Sep 17 00:00:00 2001 From: Ben Ash <32777270+benashz@users.noreply.github.com> Date: Fri, 10 Dec 2021 18:07:47 -0500 Subject: [PATCH] Support clearing an identity alias' custom_metadata (#13395) * Support clearing an identity alias' custom_metadata Previously, an update to an entity alias supported updating the custom_metadata as long as the update was not empty, which makes it impossible to clear the metadata values completely. Fixes: - empty custom_metadata parameters are honoured on entity alias update - update related tests - drop dependency on mapstructure - reformat with gofumpt --- changelog/13395.txt | 3 + vault/identity_store_aliases.go | 32 ++--- vault/identity_store_aliases_test.go | 194 +++++++++++++++++++++------ 3 files changed, 172 insertions(+), 57 deletions(-) create mode 100644 changelog/13395.txt diff --git a/changelog/13395.txt b/changelog/13395.txt new file mode 100644 index 000000000..d8f2e71a5 --- /dev/null +++ b/changelog/13395.txt @@ -0,0 +1,3 @@ +```release-note:improvement +core/identity: Support updating an alias' `custom_metadata` to be empty. +``` diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go index 893eba58b..b717f83ea 100644 --- a/vault/identity_store_aliases.go +++ b/vault/identity_store_aliases.go @@ -8,18 +8,20 @@ import ( "github.com/golang/protobuf/ptypes" "github.com/hashicorp/go-multierror" "github.com/hashicorp/go-secure-stdlib/strutil" + "github.com/hashicorp/vault/helper/identity" "github.com/hashicorp/vault/helper/namespace" "github.com/hashicorp/vault/helper/storagepacker" "github.com/hashicorp/vault/sdk/framework" "github.com/hashicorp/vault/sdk/logical" - "github.com/mitchellh/mapstructure" ) -const maxCustomMetadataKeys = 64 -const maxCustomMetadataKeyLength = 128 -const maxCustomMetadataValueLength = 512 -const customMetadataValidationErrorPrefix = "custom_metadata validation failed" +const ( + maxCustomMetadataKeys = 64 + maxCustomMetadataKeyLength = 128 + maxCustomMetadataValueLength = 512 + customMetadataValidationErrorPrefix = "custom_metadata validation failed" +) // aliasPaths returns the API endpoints to operate on aliases. // Following are the paths supported: @@ -138,10 +140,7 @@ func (i *IdentityStore) handleAliasCreateUpdate() framework.OperationFunc { customMetadata := make(map[string]string) data, customMetadataExists := d.GetOk("custom_metadata") if customMetadataExists { - err = mapstructure.Decode(data, &customMetadata) - if err != nil { - return nil, err - } + customMetadata = data.(map[string]string) } // Get entity id @@ -151,11 +150,9 @@ func (i *IdentityStore) handleAliasCreateUpdate() framework.OperationFunc { canonicalID = d.Get("entity_id").(string) } - //validate customMetadata if provided + // validate customMetadata if provided if len(customMetadata) != 0 { - - err := validateCustomMetadata(customMetadata) - if err != nil { + if err := validateCustomMetadata(customMetadata); err != nil { return nil, err } } @@ -178,8 +175,11 @@ func (i *IdentityStore) handleAliasCreateUpdate() framework.OperationFunc { if alias.NamespaceID != ns.ID { return logical.ErrorResponse("cannot modify aliases across namespaces"), logical.ErrPermissionDenied } + if !customMetadataExists { + customMetadata = alias.CustomMetadata + } switch { - case mountAccessor == "" && name == "" && len(customMetadata) == 0: + case mountAccessor == "" && name == "": // Just a canonical ID update, maybe if canonicalID == "" { // Nothing to do, so be idempotent @@ -187,16 +187,12 @@ func (i *IdentityStore) handleAliasCreateUpdate() framework.OperationFunc { } name = alias.Name mountAccessor = alias.MountAccessor - customMetadata = alias.CustomMetadata case mountAccessor == "": // No change to mount accessor mountAccessor = alias.MountAccessor case name == "": // No change to mount name name = alias.Name - case len(customMetadata) == 0: - // No change to custom metadata - customMetadata = alias.CustomMetadata default: // mountAccessor, name and customMetadata provided } diff --git a/vault/identity_store_aliases_test.go b/vault/identity_store_aliases_test.go index de3db6956..6c0f7a0f0 100644 --- a/vault/identity_store_aliases_test.go +++ b/vault/identity_store_aliases_test.go @@ -355,55 +355,173 @@ func TestIdentityStore_AliasRegister(t *testing.T) { } func TestIdentityStore_AliasUpdate(t *testing.T) { - var err error - var resp *logical.Response ctx := namespace.RootContext(nil) is, githubAccessor, _ := testIdentityStoreWithGithubAuth(ctx, t) - aliasData := map[string]interface{}{ - "name": "testaliasname", - "mount_accessor": githubAccessor, + tests := []struct { + name string + createData map[string]interface{} + updateData map[string]interface{} + }{ + { + name: "noop", + createData: map[string]interface{}{ + "name": "noop", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{ + "foo": "baz", + "bar": "qux", + }, + }, + updateData: map[string]interface{}{ + "name": "noop", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{ + "foo": "baz", + "bar": "qux", + }, + }, + }, + { + name: "no-custom-metadata", + createData: map[string]interface{}{ + "name": "no-custom-metadata", + "mount_accessor": githubAccessor, + }, + updateData: map[string]interface{}{ + "name": "no-custom-metadata", + "mount_accessor": githubAccessor, + }, + }, + { + name: "update-name-custom-metadata", + createData: map[string]interface{}{ + "name": "update-name-custom-metadata", + "mount_accessor": githubAccessor, + "custom_metadata": make(map[string]string), + }, + updateData: map[string]interface{}{ + "name": "update-name-custom-metadata-2", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{ + "bar": "qux", + }, + }, + }, + { + name: "update-name", + createData: map[string]interface{}{ + "name": "update-name", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{ + "foo": "baz", + }, + }, + updateData: map[string]interface{}{ + "name": "update-name-2", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{ + "foo": "baz", + }, + }, + }, + { + name: "update-metadata", + createData: map[string]interface{}{ + "name": "update-metadata", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{ + "foo": "bar", + }, + }, + updateData: map[string]interface{}{ + "name": "update-metadata", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{ + "foo": "baz", + "bar": "qux", + }, + }, + }, + { + name: "clear-metadata", + createData: map[string]interface{}{ + "name": "clear", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{ + "foo": "bar", + }, + }, + updateData: map[string]interface{}{ + "name": "clear", + "mount_accessor": githubAccessor, + "custom_metadata": map[string]string{}, + }, + }, } - aliasReq := &logical.Request{ - Operation: logical.UpdateOperation, - Path: "entity-alias", - Data: aliasData, + handleRequest := func(t *testing.T, req *logical.Request) *logical.Response { + t.Helper() + resp, err := is.HandleRequest(ctx, req) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("err:%v resp:%#v", err, resp) + } + return resp } - // This will create an alias and a corresponding entity - resp, err = is.HandleRequest(ctx, aliasReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%v resp:%#v", err, resp) - } - aliasID := resp.Data["id"].(string) - customMetadata := make(map[string]string) - customMetadata["foo"] = "abc" - - updateData := map[string]interface{}{ - "name": "updatedaliasname", - "mount_accessor": githubAccessor, - "custom_metadata": customMetadata, + respDefaults := map[string]interface{}{ + "custom_metadata": map[string]string{}, } - aliasReq.Data = updateData - aliasReq.Path = "entity-alias/id/" + aliasID - resp, err = is.HandleRequest(ctx, aliasReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%v resp:%#v", err, resp) + checkValues := func(t *testing.T, reqData, respData map[string]interface{}) { + t.Helper() + expected := make(map[string]interface{}) + for k := range reqData { + expected[k] = reqData[k] + } + + for k := range respDefaults { + if _, ok := expected[k]; !ok { + expected[k] = respDefaults[k] + } + } + + actual := make(map[string]interface{}, len(expected)) + for k := range expected { + actual[k] = respData[k] + } + + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("expected data %#v, actual %#v", expected, actual) + } } - aliasReq.Operation = logical.ReadOperation - resp, err = is.HandleRequest(ctx, aliasReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("err:%v resp:%#v", err, resp) - } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resp := handleRequest(t, &logical.Request{ + Operation: logical.UpdateOperation, + Path: "entity-alias", + Data: tt.createData, + }) - if resp.Data["name"] != "updatedaliasname" { - t.Fatalf("failed to update alias information; \n response data: %#v\n", resp.Data) - } - if !reflect.DeepEqual(resp.Data["custom_metadata"], customMetadata) { - t.Fatalf("failed to update alias information; \n response data: %#v\n", resp.Data) + readReq := &logical.Request{ + Operation: logical.ReadOperation, + Path: "entity-alias/id/" + resp.Data["id"].(string), + } + + // check create + resp = handleRequest(t, readReq) + checkValues(t, tt.createData, resp.Data) + + // check update + resp = handleRequest(t, &logical.Request{ + Operation: logical.UpdateOperation, + Path: readReq.Path, + Data: tt.updateData, + }) + resp = handleRequest(t, readReq) + checkValues(t, tt.updateData, resp.Data) + }) } } @@ -489,7 +607,6 @@ func TestIdentityStore_AliasMove_DuplicateAccessor(t *testing.T) { if resp == nil || !resp.IsError() { t.Fatalf("expected an error as alias on the github accessor exists for testentity1") } - } // Test that the alias cannot be changed to a mount for which @@ -563,7 +680,6 @@ func TestIdentityStore_AliasUpdate_DuplicateAccessor(t *testing.T) { if resp == nil || !resp.IsError() { t.Fatalf("expected an error as an alias on the github accessor already exists for testentity") } - } // Test that alias creation fails if an alias for the specified mount