Backport pr 12747 1.6 (#12838)

* [VAULT-3252] Disallow alias creation if entity/accessor combination exists (#12747) * Disallow alias creation if entity/accessor combination exists * Add changelog * Address review comments * Add handling to aliasUpdate, some field renaming * Update tests to work under new entity-alias constraint * Add check to entity merge, other review fixes * Log duplicated accessors only once * Fix flaky test * Add note about new constraint to docs * Update entity merge warn log * Add doc update
hashicorp · Oct 14, 2021 · b01189a · b01189a
1 parent 9070bc3
commit b01189a
Show file tree

Hide file tree

Showing 9 changed files with 403 additions and 51 deletions.
diff --git a/changelog/12747.txt b/changelog/12747.txt
@@ -0,0 +1,3 @@
+```release-note:bug
+core/identity: Disallow entity alias creation/update if a conflicting alias exists for the target entity and mount combination
+```
diff --git a/vault/core_metrics_test.go b/vault/core_metrics_test.go
@@ -2,6 +2,7 @@ package vault
 
 import (
 	"errors"
+	"sort"
 	"strings"
 	"testing"
 	"time"
@@ -254,7 +255,7 @@ func metricLabelsMatch(t *testing.T, actual []metrics.Label, expected map[string
 
 func TestCoreMetrics_EntityGauges(t *testing.T) {
 	ctx := namespace.RootContext(nil)
-	is, ghAccessor, core := testIdentityStoreWithGithubAuth(ctx, t)
+	is, ghAccessor, upAccessor, core := testIdentityStoreWithGithubUserpassAuth(ctx, t)
 
 	// Create an entity
 	alias1 := &logical.Alias{
@@ -273,12 +274,11 @@ func TestCoreMetrics_EntityGauges(t *testing.T) {
 		Operation: logical.UpdateOperation,
 		Path:      "entity-alias",
 		Data: map[string]interface{}{
-			"name":           "githubuser2",
+			"name":           "userpassuser",
 			"canonical_id":   entity.ID,
-			"mount_accessor": ghAccessor,
+			"mount_accessor": upAccessor,
 		},
 	}
-
 	resp, err := is.HandleRequest(ctx, registerReq)
 	if err != nil || (resp != nil && resp.IsError()) {
 		t.Fatalf("err:%v resp:%#v", err, resp)
@@ -307,18 +307,38 @@ func TestCoreMetrics_EntityGauges(t *testing.T) {
 		t.Fatalf("err: %v", err)
 	}
 
-	if len(glv) != 1 {
+	if len(glv) != 2 {
 		t.Fatalf("Wrong number of gauges %v, expected %v", len(glv), 1)
 	}
 
-	if glv[0].Value != 2.0 {
-		t.Errorf("Alias count %v, expected %v", glv[0].Value, 2.0)
+	if glv[0].Value != 1.0 {
+		t.Errorf("Alias count %v, expected %v", glv[0].Value, 1.0)
+	}
+
+	if glv[1].Value != 1.0 {
+		t.Errorf("Alias count %v, expected %v", glv[0].Value, 1.0)
 	}
 
+	// Sort both metrics.Label slices by Name, causing the Label
+	// with Name auth_method to be first in both arrays
+	sort.Slice(glv[0].Labels, func(i, j int) bool { return glv[0].Labels[i].Name < glv[0].Labels[j].Name })
+	sort.Slice(glv[1].Labels, func(i, j int) bool { return glv[1].Labels[i].Name < glv[1].Labels[j].Name })
+
+	// Sort the GaugeLabelValues slice by the Value of the first metric,
+	// in this case auth_method, in each metrics.Label slice
+	sort.Slice(glv, func(i, j int) bool { return glv[i].Labels[0].Value < glv[j].Labels[0].Value })
+
 	metricLabelsMatch(t, glv[0].Labels,
 		map[string]string{
 			"namespace":   "root",
 			"auth_method": "github",
 			"mount_point": "auth/github/",
 		})
+
+	metricLabelsMatch(t, glv[1].Labels,
+		map[string]string{
+			"namespace":   "root",
+			"auth_method": "userpass",
+			"mount_point": "auth/userpass/",
+		})
 }
diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go
@@ -231,6 +231,12 @@ func (i *IdentityStore) handleAliasCreate(ctx context.Context, req *logical.Requ
 		}
 	}
 
+	for _, currentAlias := range entity.Aliases {
+		if currentAlias.MountAccessor == mountAccessor {
+			return logical.ErrorResponse("Alias already exists for requested entity and mount accessor"), nil
+		}
+	}
+
 	entity.Aliases = append(entity.Aliases, alias)
 
 	// ID creation and other validations; This is more useful for new entities
@@ -276,6 +282,29 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ
 
 	alias.LastUpdateTime = ptypes.TimestampNow()
 
+	// Get our current entity, which may be the same as the new one if the
+	// canonical ID hasn't changed
+	currentEntity, err := i.MemDBEntityByAliasID(alias.ID, true)
+	if err != nil {
+		return nil, err
+	}
+	if currentEntity == nil {
+		return logical.ErrorResponse("given alias is not associated with an entity"), nil
+	}
+
+	if currentEntity.NamespaceID != alias.NamespaceID {
+		return logical.ErrorResponse("alias and entity do not belong to the same namespace"), logical.ErrPermissionDenied
+	}
+
+	// If the accessor is being changed but the entity is not, check if the entity
+	// already has an alias corresponding to the new accessor
+	if mountAccessor != alias.MountAccessor && (canonicalID == "" || canonicalID == alias.CanonicalID) {
+		for _, currentAlias := range currentEntity.Aliases {
+			if currentAlias.MountAccessor == mountAccessor {
+				return logical.ErrorResponse("Alias cannot be updated as the entity already has an alias for the given 'mount_accessor' "), nil
+			}
+		}
+	}
 	// If we're changing one or the other or both of these, make sure that
 	// there isn't a matching alias already, and make sure it's in the same
 	// namespace.
@@ -306,19 +335,6 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ
 		alias.MountAccessor = mountAccessor
 	}
 
-	// Get our current entity, which may be the same as the new one if the
-	// canonical ID hasn't changed
-	currentEntity, err := i.MemDBEntityByAliasID(alias.ID, true)
-	if err != nil {
-		return nil, err
-	}
-	if currentEntity == nil {
-		return logical.ErrorResponse("given alias is not associated with an entity"), nil
-	}
-	if currentEntity.NamespaceID != alias.NamespaceID {
-		return logical.ErrorResponse("alias associated with an entity in a different namespace"), logical.ErrPermissionDenied
-	}
-
 	newEntity := currentEntity
 	if canonicalID != "" && canonicalID != alias.CanonicalID {
 		newEntity, err = i.MemDBEntityByID(canonicalID, true)
@@ -332,7 +348,14 @@ func (i *IdentityStore) handleAliasUpdate(ctx context.Context, req *logical.Requ
 			return logical.ErrorResponse("given 'canonical_id' associated with entity in a different namespace from the alias"), logical.ErrPermissionDenied
 		}
 
-		// Update the canonical ID value and move it from the current enitity to the new one
+		// Check if the entity the alias is being updated to, already has an alias for the mount
+		for _, alias := range newEntity.Aliases {
+			if alias.MountAccessor == mountAccessor {
+				return logical.ErrorResponse("Alias cannot be updated as the given entity already has an alias for this mount "), nil
+			}
+		}
+
+		// Update the canonical ID value and move it from the current entity to the new one
 		alias.CanonicalID = newEntity.ID
 		newEntity.Aliases = append(newEntity.Aliases, alias)
 		for aliasIndex, item := range currentEntity.Aliases {

diff --git a/vault/identity_store_aliases_test.go b/vault/identity_store_aliases_test.go
@@ -399,6 +399,218 @@ func TestIdentityStore_AliasUpdate(t *testing.T) {
 	}
 }
 
+// Test to check that the alias cannot be updated with a new entity
+// which already has an alias for the mount on the alias to be updated
+func TestIdentityStore_AliasMove_DuplicateAccessor(t *testing.T) {
+	var err error
+	var resp *logical.Response
+	ctx := namespace.RootContext(nil)
+	is, githubAccessor, _ := testIdentityStoreWithGithubAuth(ctx, t)
+
+	// Create 2 entities and 1 alias on each, against the same github mount
+	resp, err = is.HandleRequest(ctx, &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "testentity1",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entity1ID := resp.Data["id"].(string)
+
+	alias1Data := map[string]interface{}{
+		"name":           "testaliasname1",
+		"mount_accessor": githubAccessor,
+		"canonical_id":   entity1ID,
+	}
+
+	aliasReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity-alias",
+		Data:      alias1Data,
+	}
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	resp, err = is.HandleRequest(ctx, &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "testentity2",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entity2ID := resp.Data["id"].(string)
+
+	alias2Data := map[string]interface{}{
+		"name":           "testaliasname2",
+		"mount_accessor": githubAccessor,
+		"canonical_id":   entity2ID,
+	}
+
+	aliasReq.Data = alias2Data
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+	alias2ID := resp.Data["id"].(string)
+
+	// Attempt to update the second alias to point to the first entity
+	updateData := map[string]interface{}{
+		"canonical_id": entity1ID,
+	}
+
+	aliasReq.Data = updateData
+	aliasReq.Path = "entity-alias/id/" + alias2ID
+	resp, err = is.HandleRequest(ctx, aliasReq)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	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
+// the entity already has an alias
+func TestIdentityStore_AliasUpdate_DuplicateAccessor(t *testing.T) {
+	var err error
+	var resp *logical.Response
+	ctx := namespace.RootContext(nil)
+
+	is, ghAccessor, upAccessor, _ := testIdentityStoreWithGithubUserpassAuth(ctx, t)
+
+	// Create 1 entity and 2 aliases on it, one for each mount
+	resp, err = is.HandleRequest(ctx, &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+		Data: map[string]interface{}{
+			"name": "testentity",
+		},
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entityID := resp.Data["id"].(string)
+
+	alias1Data := map[string]interface{}{
+		"name":           "testaliasname1",
+		"mount_accessor": ghAccessor,
+		"canonical_id":   entityID,
+	}
+
+	aliasReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity-alias",
+		Data:      alias1Data,
+	}
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	alias2Data := map[string]interface{}{
+		"name":           "testaliasname2",
+		"mount_accessor": upAccessor,
+		"canonical_id":   entityID,
+	}
+
+	aliasReq.Data = alias2Data
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+	alias2ID := resp.Data["id"].(string)
+
+	// Attempt to update the userpass mount to point to the github mount
+	updateData := map[string]interface{}{
+		"mount_accessor": ghAccessor,
+	}
+
+	aliasReq.Data = updateData
+	aliasReq.Path = "entity-alias/id/" + alias2ID
+	resp, err = is.HandleRequest(ctx, aliasReq)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	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
+// and entity has already been created
+func TestIdentityStore_AliasCreate_DuplicateAccessor(t *testing.T) {
+	var err error
+	var resp *logical.Response
+	ctx := namespace.RootContext(nil)
+	is, githubAccessor, _ := testIdentityStoreWithGithubAuth(ctx, t)
+
+	resp, err = is.HandleRequest(ctx, &logical.Request{
+		Path:      "entity",
+		Operation: logical.UpdateOperation,
+	})
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+	entityID := resp.Data["id"].(string)
+
+	aliasData := map[string]interface{}{
+		"name":           "testaliasname",
+		"mount_accessor": githubAccessor,
+		"canonical_id":   entityID,
+	}
+
+	aliasReq := &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity-alias",
+		Data:      aliasData,
+	}
+
+	// This will create an alias against the requested entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("err:%v resp:%#v", err, resp)
+	}
+
+	aliasData["name"] = "testaliasname2"
+	aliasReq = &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "entity-alias",
+		Data:      aliasData,
+	}
+
+	// This will try to create a new alias with the same accessor and entity
+	resp, err = is.HandleRequest(ctx, aliasReq)
+
+	if err != nil {
+		t.Fatal(err)
+	}
+	if resp == nil || !resp.IsError() {
+		t.Fatalf("expected an error as alias already exists for this accessor and entity")
+	}
+}
+
 func TestIdentityStore_AliasUpdate_ByID(t *testing.T) {
 	var err error
 	var resp *logical.Response