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

Fix issue caused by group-creation forwarding. #29559

Merged
merged 3 commits into from
Feb 12, 2025
Merged
Show file tree
Hide file tree
Changes from all 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
5 changes: 2 additions & 3 deletions vault/identity_store_conflicts.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ type duplicateReportingErrorResolver struct {
// when in case-sensitive mode.
//
// Since this is only ever called from `load*` methods on IdentityStore during
// an unseal we can assume that it's all from a single goroutine and does'nt
// an unseal we can assume that it's all from a single goroutine and doesn't
// need locking.
seenEntities map[string][]*identity.Entity
seenGroups map[string][]*identity.Group
Expand Down Expand Up @@ -316,8 +316,7 @@ type Warner interface {
Warn(msg string, args ...interface{})
}

// TODO set this correctly.
const identityDuplicateReportUrl = "https://developer.hashicorp.com/vault/docs/upgrading/identity-deduplication"
const identityDuplicateReportUrl = "https://developer.hashicorp.com/vault/docs/upgrading/deduplication"

func (r *duplicateReportingErrorResolver) LogReport(log Warner) {
report := r.Report()
Expand Down
4 changes: 2 additions & 2 deletions vault/identity_store_conflicts_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func TestDuplicateReportingErrorResolver(t *testing.T) {
}

expectReport := `
DUPLICATES DETECTED, see following logs for details and refer to https://developer.hashicorp.com/vault/docs/upgrading/identity-deduplication for resolution.:
DUPLICATES DETECTED, see following logs for details and refer to https://developer.hashicorp.com/vault/docs/upgrading/deduplication for resolution.:
1 different-case local entity alias duplicates found (potential security risk):
local entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "local-mount" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" canonical_id="11111111-0000-0000-0000-000000000009" force_deduplication="would merge others into this entity"
local entity-alias "different-CASE-ALIAS-1" with mount accessor "local-mount" duplicates 1 others: id="00000000-0000-0000-0000-000000000010" canonical_id="11111111-0000-0000-0000-000000000010" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000009"
Expand Down Expand Up @@ -99,7 +99,7 @@ group "DIFFERENT-CASE-DUPE-1" with namespace ID "root" duplicates 1 others: id="
group "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" force_deduplication="would not rename"
group "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000005"
end of group duplicates:
end of identity duplicate report, refer to https://developer.hashicorp.com/vault/docs/upgrading/identity-deduplication for resolution.:
end of identity duplicate report, refer to https://developer.hashicorp.com/vault/docs/upgrading/deduplication for resolution.:
`

// Create a new errorResolver
Expand Down
12 changes: 10 additions & 2 deletions vault/identity_store_group_aliases.go
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,12 @@ func groupAliasPaths(i *IdentityStore) []*framework.Path {
},
},

Callbacks: map[logical.Operation]framework.OperationFunc{
logical.UpdateOperation: i.pathGroupAliasRegister(),
Operations: map[logical.Operation]framework.OperationHandler{
logical.UpdateOperation: &framework.PathOperation{
Callback: i.pathGroupAliasRegister(),
ForwardPerformanceStandby: true,
ForwardPerformanceSecondary: true,
},
},

HelpSynopsis: strings.TrimSpace(groupAliasHelp["group-alias"][0]),
Expand Down Expand Up @@ -85,6 +89,8 @@ func groupAliasPaths(i *IdentityStore) []*framework.Path {
DisplayAttrs: &framework.DisplayAttributes{
OperationVerb: "update",
},
ForwardPerformanceStandby: true,
ForwardPerformanceSecondary: true,
},
logical.ReadOperation: &framework.PathOperation{
Callback: i.pathGroupAliasIDRead(),
Expand All @@ -97,6 +103,8 @@ func groupAliasPaths(i *IdentityStore) []*framework.Path {
DisplayAttrs: &framework.DisplayAttributes{
OperationVerb: "delete",
},
ForwardPerformanceStandby: true,
ForwardPerformanceSecondary: true,
},
},

Expand Down
11 changes: 11 additions & 0 deletions vault/identity_store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1790,12 +1790,23 @@ func TestIdentityStoreLoadingDuplicateReporting(t *testing.T) {
// many of these cases and seems strange to encode in a test that we want
// broken behavior!
numDupes := make(map[string]int)
uniqueIDs := make(map[string]struct{})
duplicateCountRe := regexp.MustCompile(`(\d+) (different-case( local)? entity alias|entity|group) duplicates found`)
// Be sure not to match attributes like alias_id= because there are dupes
// there. The report lines we care about always have a space before the id
// pair.
propsRe := regexp.MustCompile(`\s(id=(\S+))`)
for _, log := range unsealLogs {
if matches := duplicateCountRe.FindStringSubmatch(log); len(matches) >= 3 {
num, _ := strconv.Atoi(matches[1])
numDupes[matches[2]] = num
}
if propMatches := propsRe.FindStringSubmatch(log); len(propMatches) >= 3 {
artifactID := propMatches[2]
require.NotContains(t, uniqueIDs, artifactID,
"duplicate ID reported in logs for different artifacts")
uniqueIDs[artifactID] = struct{}{}
}
}
t.Logf("numDupes: %v", numDupes)
wantAliases, wantLocalAliases, wantEntities, wantGroups := identityStoreDuplicateReportTestWantDuplicateCounts()
Expand Down
8 changes: 0 additions & 8 deletions vault/identity_store_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -2035,14 +2035,6 @@ func (i *IdentityStore) UpsertGroupInTxn(ctx context.Context, txn *memdb.Txn, gr
return fmt.Errorf("group is nil")
}

g, err := i.MemDBGroupByName(ctx, group.Name, true)
Copy link
Contributor

Choose a reason for hiding this comment

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

🤔 I wonder if this would have worked if we used the txn that's passed into this method...

Copy link
Member Author

Choose a reason for hiding this comment

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

It would not have. The problem here is the lines below where we set the incoming group.ID to be the same as the existing one. That meant that when there are duplicate groups in storage we end up loading the second one over the first, but switching it to have the first one's ID so now you can't even lookup the original by ID and the one you do get is a strange hybrid of two different groups in storage...

if err != nil {
return err
}
if g != nil {
group.ID = g.ID
}

// Increment the modify index of the group
group.ModifyIndex++

Expand Down
Loading