From 68999c87500337c86b6eaf6abd83db036d2602a4 Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Thu, 9 Jun 2022 14:17:51 -0400 Subject: [PATCH 1/5] return bad request for identity group cycle detection --- vault/identity_store_groups.go | 4 ++ vault/identity_store_groups_test.go | 89 +++++++++++++++++++++++++++++ vault/identity_store_util.go | 5 +- 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/vault/identity_store_groups.go b/vault/identity_store_groups.go index 4a8b692737e63..797c2b53974f2 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(err.Error(), 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..6f31d301d552b 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" @@ -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 } } From 342d611ce3dabd1c6cea5ad6282d5dd9c75f191b Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Thu, 9 Jun 2022 14:56:43 -0400 Subject: [PATCH 2/5] add changelog entry --- changelog/15912.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/15912.txt diff --git a/changelog/15912.txt b/changelog/15912.txt new file mode 100644 index 0000000000000..af85c4254c3b2 --- /dev/null +++ b/changelog/15912.txt @@ -0,0 +1,3 @@ +```release-note:improvement +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 +``` From 5e8e35bc28a3e3eb84121d184dfd9cda80680b62 Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Thu, 9 Jun 2022 14:58:09 -0400 Subject: [PATCH 3/5] use change release note instead of improvement --- changelog/15912.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/15912.txt b/changelog/15912.txt index af85c4254c3b2..391d7353b6280 100644 --- a/changelog/15912.txt +++ b/changelog/15912.txt @@ -1,3 +1,3 @@ -```release-note:improvement +```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 ``` From aeb44350e927b24e0c74479538d42a3af9a83fea Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Thu, 9 Jun 2022 14:59:16 -0400 Subject: [PATCH 4/5] fix err reference --- vault/identity_store_groups.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/identity_store_groups.go b/vault/identity_store_groups.go index 797c2b53974f2..224521d8c30ef 100644 --- a/vault/identity_store_groups.go +++ b/vault/identity_store_groups.go @@ -255,7 +255,7 @@ 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(err.Error(), errCycleDetectedPrefix) { + if errStr := err.Error(); strings.HasPrefix(errStr, errCycleDetectedPrefix) { return logical.ErrorResponse(errStr), nil } From 5c2c6c889a7cc84e6527407ac2c30fe4b6c89cb9 Mon Sep 17 00:00:00 2001 From: Chris Capurso <1036769+ccapurso@users.noreply.github.com> Date: Thu, 9 Jun 2022 16:07:40 -0400 Subject: [PATCH 5/5] fix TestIdentityStore_GroupHierarchyCases --- vault/identity_store_groups_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/vault/identity_store_groups_test.go b/vault/identity_store_groups_test.go index 6f31d301d552b..2e138e264f545 100644 --- a/vault/identity_store_groups_test.go +++ b/vault/identity_store_groups_test.go @@ -1253,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") }