diff --git a/changelog/15912.txt b/changelog/15912.txt new file mode 100644 index 0000000000000..391d7353b6280 --- /dev/null +++ b/changelog/15912.txt @@ -0,0 +1,3 @@ +```release-note:change +identity: a request to `/identity/group` that includes `member_group_ids` that contains a cycle will now be responded to with a 400 rather than 500 +``` diff --git a/vault/identity_store_groups.go b/vault/identity_store_groups.go index 4a8b692737e63..224521d8c30ef 100644 --- a/vault/identity_store_groups.go +++ b/vault/identity_store_groups.go @@ -255,6 +255,10 @@ func (i *IdentityStore) handleGroupUpdateCommon(ctx context.Context, req *logica err = i.sanitizeAndUpsertGroup(ctx, group, nil, memberGroupIDs) if err != nil { + if errStr := err.Error(); strings.HasPrefix(errStr, errCycleDetectedPrefix) { + return logical.ErrorResponse(errStr), nil + } + return nil, err } diff --git a/vault/identity_store_groups_test.go b/vault/identity_store_groups_test.go index 4ae85ff048fd9..2e138e264f545 100644 --- a/vault/identity_store_groups_test.go +++ b/vault/identity_store_groups_test.go @@ -1,6 +1,7 @@ package vault import ( + "fmt" "reflect" "sort" "strings" @@ -1252,7 +1253,7 @@ func TestIdentityStore_GroupHierarchyCases(t *testing.T) { groupUpdateReq.Data = kubeGroupData kubeGroupData["member_group_ids"] = []string{engGroupID} resp, err = is.HandleRequest(ctx, groupUpdateReq) - if err == nil { + if err != nil || resp == nil || !resp.IsError() { t.Fatalf("expected an error response") } @@ -1391,3 +1392,91 @@ func TestIdentityStore_GroupHierarchyCases(t *testing.T) { t.Fatalf("bad: length of inheritedGroups; expected: 0, actual: %d", len(inheritedGroups)) } } + +func TestIdentityStore_GroupCycleDetection(t *testing.T) { + c, _, _ := TestCoreUnsealed(t) + ctx := namespace.RootContext(nil) + + group1Name := "group1" + group2Name := "group2" + group3Name := "group3" + + resp, err := c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "group", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "name": group1Name, + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("failed to create group %q, err: %v, resp: %#v", group1Name, err, resp) + } + + group1Id := resp.Data["id"].(string) + + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "group", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "name": group2Name, + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("failed to create group %q, err: %v, resp: %#v", group2Name, err, resp) + } + + group2Id := resp.Data["id"].(string) + + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "group", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "name": group3Name, + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("failed to create group %q, err: %v, resp: %#v", group3Name, err, resp) + } + + group3Id := resp.Data["id"].(string) + + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "group", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "name": group1Name, + "member_group_ids": []string{}, + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("failed to update group %q, err: %v, resp: %#v", group1Name, err, resp) + } + + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "group", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "name": group2Name, + "member_group_ids": []string{group3Id}, + }, + }) + if err != nil || (resp != nil && resp.IsError()) { + t.Fatalf("failed to update group %q, err: %v, resp: %#v", group2Name, err, resp) + } + + resp, err = c.identityStore.HandleRequest(ctx, &logical.Request{ + Path: "group", + Operation: logical.UpdateOperation, + Data: map[string]interface{}{ + "name": group3Name, + "member_group_ids": []string{group1Id, group2Id}, + }, + }) + + if err != nil || resp == nil { + t.Fatalf("unexpected group update error for group %q, err: %v, resp: %#v", group3Name, err, resp) + } + if !resp.IsError() || resp.Error().Error() != fmt.Sprintf("%s %q", errCycleDetectedPrefix, group2Id) { + t.Fatalf("expected update to group %q to fail due to cycle, resp: %#v", group3Id, resp) + } +} diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index b6baea019fa21..277460aefe2b8 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -24,6 +24,7 @@ import ( var ( errDuplicateIdentityName = errors.New("duplicate identity name") + errCycleDetectedPrefix = "cyclic relationship detected for member group ID" tmpSuffix = ".tmp" ) @@ -1578,7 +1579,7 @@ func (i *IdentityStore) sanitizeAndUpsertGroup(ctx context.Context, group *ident return fmt.Errorf("failed to perform cyclic relationship detection for member group ID %q", memberGroupID) } if cycleDetected { - return fmt.Errorf("cyclic relationship detected for member group ID %q", memberGroupID) + return fmt.Errorf("%s %q", errCycleDetectedPrefix, memberGroupID) } } @@ -2157,7 +2158,7 @@ func (i *IdentityStore) detectCycleDFS(visited map[string]bool, startingGroupID, return false, fmt.Errorf("failed to perform cycle detection at member group ID %q", memberGroup.ID) } if cycleDetected { - return true, fmt.Errorf("cycle detected at member group ID %q", memberGroup.ID) + return true, nil } }