diff --git a/changelog/29325.txt b/changelog/29325.txt new file mode 100644 index 0000000000..5c213fea0a --- /dev/null +++ b/changelog/29325.txt @@ -0,0 +1,4 @@ +```release-note:improvement +identity: Added reporting in Vault logs during unseal to help identify any +duplicate identify resources in storage. +``` diff --git a/vault/identity_store_conflicts.go b/vault/identity_store_conflicts.go index 8495af1a25..23cabcf10e 100644 --- a/vault/identity_store_conflicts.go +++ b/vault/identity_store_conflicts.go @@ -6,7 +6,11 @@ package vault import ( "context" "errors" + "fmt" + "sort" + "strings" + "github.com/hashicorp/go-hclog" log "github.com/hashicorp/go-hclog" "github.com/hashicorp/vault/helper/identity" ) @@ -83,3 +87,277 @@ func (r *errorResolver) ResolveAliases(ctx context.Context, parent *identity.Ent return errDuplicateIdentityName } + +// duplicateReportingErrorResolver collects duplicate information and optionally +// logs a report on all the duplicates. We don't embed an errorResolver here +// because we _don't_ want it's side effect of warning on just some duplicates +// as we go as that's confusing when we have a more comprehensive report. The +// only other behavior it has is to return a constant error which we can just do +// ourselves. +type duplicateReportingErrorResolver struct { + // seen* track the unique factors for each identity artifact, so + // that we can report on any duplication including different-case duplicates + // 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 + // need locking. + seenEntities map[string][]*identity.Entity + seenGroups map[string][]*identity.Group + seenAliases map[string][]*identity.Alias + seenLocalAliases map[string][]*identity.Alias + logger hclog.Logger +} + +func newDuplicateReportingErrorResolver(logger hclog.Logger) *duplicateReportingErrorResolver { + return &duplicateReportingErrorResolver{ + seenEntities: make(map[string][]*identity.Entity), + seenGroups: make(map[string][]*identity.Group), + seenAliases: make(map[string][]*identity.Alias), + seenLocalAliases: make(map[string][]*identity.Alias), + logger: logger, + } +} + +func (r *duplicateReportingErrorResolver) ResolveEntities(ctx context.Context, existing, duplicate *identity.Entity) error { + entityKey := fmt.Sprintf("%s/%s", duplicate.NamespaceID, strings.ToLower(duplicate.Name)) + r.seenEntities[entityKey] = append(r.seenEntities[entityKey], duplicate) + return errDuplicateIdentityName +} + +func (r *duplicateReportingErrorResolver) ResolveGroups(ctx context.Context, existing, duplicate *identity.Group) error { + groupKey := fmt.Sprintf("%s/%s", duplicate.NamespaceID, strings.ToLower(duplicate.Name)) + r.seenGroups[groupKey] = append(r.seenGroups[groupKey], duplicate) + return errDuplicateIdentityName +} + +func (r *duplicateReportingErrorResolver) ResolveAliases(ctx context.Context, parent *identity.Entity, existing, duplicate *identity.Alias) error { + aliasKey := fmt.Sprintf("%s/%s", duplicate.MountAccessor, strings.ToLower(duplicate.Name)) + if duplicate.Local { + r.seenLocalAliases[aliasKey] = append(r.seenLocalAliases[aliasKey], duplicate) + } else { + r.seenAliases[aliasKey] = append(r.seenAliases[aliasKey], duplicate) + } + return errDuplicateIdentityName +} + +type identityDuplicateReportEntry struct { + artifactType string + scope string + name string + id string + canonicalID string + resolutionHint string + index int // we care about preserving load order in reporting + numOthers int +} + +type identityDuplicateReport struct { + entities []identityDuplicateReportEntry + groups []identityDuplicateReportEntry + aliases []identityDuplicateReportEntry + localAliases []identityDuplicateReportEntry + numEntityDuplicates int + numGroupDuplicates int + numAliasDuplicates int + numLocalAliasDuplicates int +} + +func (r *identityDuplicateReportEntry) Description() string { + scopeField := "namespace ID" + if r.artifactType == "entity-alias" || r.artifactType == "local entity-alias" { + scopeField = "mount accessor" + } + return fmt.Sprintf("%s %q with %s %q duplicates %d others", + r.artifactType, r.name, scopeField, r.scope, r.numOthers) +} + +// Labels returns metadata pairs suitable for passing to a logger each slice +// element corresponds alternately to a key and then a value. +func (r *identityDuplicateReportEntry) Labels() []interface{} { + args := []interface{}{"id", r.id} + if r.canonicalID != "" { + args = append(args, "canonical_id") + args = append(args, r.canonicalID) + } + if r.resolutionHint != "" { + args = append(args, "force_deduplication") + args = append(args, r.resolutionHint) + } + return args +} + +func (r *duplicateReportingErrorResolver) Report() identityDuplicateReport { + var report identityDuplicateReport + + for _, entities := range r.seenEntities { + if len(entities) <= 1 { + // Fast path, skip non-duplicates + continue + } + report.numEntityDuplicates++ + // We don't care if it's an exact match or not for entities since we'll + // rename in either case when we force a de-dupe. + for idx, entity := range entities { + r := identityDuplicateReportEntry{ + artifactType: "entity", + scope: entity.NamespaceID, + name: entity.Name, + id: entity.ID, + index: idx, + numOthers: len(entities) - 1, + } + if idx < len(entities)-1 { + r.resolutionHint = fmt.Sprintf("would rename to %s-%s", entity.Name, entity.ID) + } else { + r.resolutionHint = "would not rename" + } + report.entities = append(report.entities, r) + } + } + sortReportEntries(report.entities) + + for _, groups := range r.seenGroups { + if len(groups) <= 1 { + // Fast path, skip non-duplicates + continue + } + report.numGroupDuplicates++ + // We don't care if it's an exact match or not for groups since we'll + // rename in either case when we force a de-dupe. + for idx, group := range groups { + r := identityDuplicateReportEntry{ + artifactType: "group", + scope: group.NamespaceID, + name: group.Name, + id: group.ID, + index: idx, + numOthers: len(groups) - 1, + } + if idx < len(groups)-1 { + r.resolutionHint = fmt.Sprintf("would rename to %s-%s", group.Name, group.ID) + } else { + r.resolutionHint = "would not rename" + } + report.groups = append(report.groups, r) + } + } + sortReportEntries(report.groups) + + reportAliases(&report, r.seenAliases, false) + reportAliases(&report, r.seenLocalAliases, true) + + return report +} + +func reportAliases(report *identityDuplicateReport, seen map[string][]*identity.Alias, local bool) { + artType := "entity-alias" + if local { + artType = "local entity-alias" + } + for _, aliases := range seen { + if len(aliases) <= 1 { + // Fast path, skip non-duplicates + continue + } + if local { + report.numLocalAliasDuplicates++ + } else { + report.numAliasDuplicates++ + } + // We can't have exact match duplicated for aliases at this point because + // the would have been merged during load. These are different-case + // duplicates that must be handled. + for idx, alias := range aliases { + r := identityDuplicateReportEntry{ + artifactType: artType, + scope: alias.MountAccessor, + name: alias.Name, + id: alias.ID, + canonicalID: alias.CanonicalID, + index: idx, + numOthers: len(aliases) - 1, + } + if idx < len(aliases)-1 { + r.resolutionHint = fmt.Sprintf("would merge into entity %s", aliases[len(aliases)-1].CanonicalID) + } else { + r.resolutionHint = "would merge others into this entity" + } + if local { + report.localAliases = append(report.localAliases, r) + } else { + report.aliases = append(report.aliases, r) + } + } + } + sortReportEntries(report.aliases) +} + +func sortReportEntries(es []identityDuplicateReportEntry) { + sort.Slice(es, func(i, j int) bool { + a, b := es[i], es[j] + if a.scope != b.scope { + return a.scope < b.scope + } + aName, bName := strings.ToLower(a.name), strings.ToLower(b.name) + if aName != bName { + return aName < bName + } + return a.index < b.index + }) +} + +// Warner is a subset of hclog.Logger that only has the Warn method to make +// testing simpler. +type Warner interface { + Warn(msg string, args ...interface{}) +} + +// TODO set this correctly. +const identityDuplicateReportUrl = "https://developer.hashicorp.com/vault/docs/upgrading/identity-deduplication" + +func (r *duplicateReportingErrorResolver) LogReport(log Warner) { + report := r.Report() + + if report.numEntityDuplicates == 0 && report.numGroupDuplicates == 0 && report.numAliasDuplicates == 0 { + return + } + + log.Warn("DUPLICATES DETECTED, see following logs for details and refer to " + + identityDuplicateReportUrl + " for resolution.") + + // Aliases first since they are most critical to resolve. Local first because + // all the rest can be ignored on a perf secondary. + if len(report.localAliases) > 0 { + log.Warn(fmt.Sprintf("%d different-case local entity alias duplicates found (potential security risk)", report.numLocalAliasDuplicates)) + for _, e := range report.localAliases { + log.Warn(e.Description(), e.Labels()...) + } + log.Warn("end of different-case local entity-alias duplicates") + } + if len(report.aliases) > 0 { + log.Warn(fmt.Sprintf("%d different-case entity alias duplicates found (potential security risk)", report.numAliasDuplicates)) + for _, e := range report.aliases { + log.Warn(e.Description(), e.Labels()...) + } + log.Warn("end of different-case entity-alias duplicates") + } + + if len(report.entities) > 0 { + log.Warn(fmt.Sprintf("%d entity duplicates found", report.numEntityDuplicates)) + for _, e := range report.entities { + log.Warn(e.Description(), e.Labels()...) + } + log.Warn("end of entity duplicates") + } + + if len(report.groups) > 0 { + log.Warn(fmt.Sprintf("%d group duplicates found", report.numGroupDuplicates)) + for _, e := range report.groups { + log.Warn(e.Description(), e.Labels()...) + } + log.Warn("end of group duplicates") + } + log.Warn("end of identity duplicate report, refer to " + + identityDuplicateReportUrl + " for resolution.") +} diff --git a/vault/identity_store_conflicts_test.go b/vault/identity_store_conflicts_test.go new file mode 100644 index 0000000000..ac757ec950 --- /dev/null +++ b/vault/identity_store_conflicts_test.go @@ -0,0 +1,180 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: BUSL-1.1 + +package vault + +import ( + "bytes" + "context" + "fmt" + "strings" + "testing" + + log "github.com/hashicorp/go-hclog" + "github.com/hashicorp/vault/helper/identity" + "github.com/stretchr/testify/require" +) + +// TestDuplicateReportingErrorResolver tests that the reporting error resolver +// correctly records, identifies, sorts and outputs information about duplicate +// entities. +func TestDuplicateReportingErrorResolver(t *testing.T) { + t.Parallel() + + entities := [][]string{ + // Some unduplicated entities in a different namespaces + {"root", "foo"}, + {"root", "bar"}, + {"admin", "foo"}, + {"developers", "BAR"}, + + // Some exact-match duplicates in different namespaces + {"root", "exact-dupe-1"}, + {"root", "exact-dupe-1"}, + {"admin", "exact-dupe-1"}, + {"admin", "exact-dupe-1"}, + + // Some different-case duplicates in different namespaces + {"root", "different-case-dupe-1"}, + {"root", "DIFFERENT-CASE-DUPE-1"}, + {"admin", "different-case-dupe-1"}, + {"admin", "DIFFERENT-case-DUPE-1"}, + {"admin", "different-case-DUPE-1"}, + } + + // Note that `local-` prefix here is used to define a mount as local as well + // as used in it's name. + aliases := [][]string{ + // Unduplicated aliases on different mounts + {"mount1", "alias1"}, + {"mount2", "alias1"}, + {"mount2", "alias2"}, + {"local-mount", "alias1"}, + + // We don't bother testing exact-match aliases since they will have been + // merged by the time they are reported) + + // Different-case aliases on different mounts (some local) + {"mount1", "different-case-alias-1"}, + {"mount1", "DIFFERENT-CASE-ALIAS-1"}, + {"mount2", "different-case-alias-1"}, + {"mount2", "DIFFERENT-CASE-ALIAS-1"}, + {"mount2", "different-CASE-ALIAS-1"}, + {"local-mount", "DIFFERENT-CASE-ALIAS-1"}, + {"local-mount", "different-CASE-ALIAS-1"}, + } + + expectReport := ` +DUPLICATES DETECTED, see following logs for details and refer to https://developer.hashicorp.com/vault/docs/upgrading/identity-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 into entity 11111111-0000-0000-0000-000000000010" +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 others into this entity" +end of different-case local entity-alias duplicates: +2 different-case entity alias duplicates found (potential security risk): +entity-alias "different-case-alias-1" with mount accessor "mount1" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" canonical_id="11111111-0000-0000-0000-000000000004" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000005" +entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "mount1" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" canonical_id="11111111-0000-0000-0000-000000000005" force_deduplication="would merge others into this entity" +entity-alias "different-case-alias-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000006" canonical_id="11111111-0000-0000-0000-000000000006" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000008" +entity-alias "DIFFERENT-CASE-ALIAS-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000007" canonical_id="11111111-0000-0000-0000-000000000007" force_deduplication="would merge into entity 11111111-0000-0000-0000-000000000008" +entity-alias "different-CASE-ALIAS-1" with mount accessor "mount2" duplicates 2 others: id="00000000-0000-0000-0000-000000000008" canonical_id="11111111-0000-0000-0000-000000000008" force_deduplication="would merge others into this entity" +end of different-case entity-alias duplicates: +4 entity duplicates found: +entity "different-case-dupe-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000010" force_deduplication="would rename to different-case-dupe-1-00000000-0000-0000-0000-000000000010" +entity "DIFFERENT-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000011" force_deduplication="would rename to DIFFERENT-case-DUPE-1-00000000-0000-0000-0000-000000000011" +entity "different-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000012" force_deduplication="would not rename" +entity "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000006" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000006" +entity "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000007" force_deduplication="would not rename" +entity "different-case-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000008" force_deduplication="would rename to different-case-dupe-1-00000000-0000-0000-0000-000000000008" +entity "DIFFERENT-CASE-DUPE-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" force_deduplication="would not rename" +entity "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000004" +entity "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" force_deduplication="would not rename" +end of entity duplicates: +4 group duplicates found: +group "different-case-dupe-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000010" force_deduplication="would rename to different-case-dupe-1-00000000-0000-0000-0000-000000000010" +group "DIFFERENT-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000011" force_deduplication="would rename to DIFFERENT-case-DUPE-1-00000000-0000-0000-0000-000000000011" +group "different-case-DUPE-1" with namespace ID "admin" duplicates 2 others: id="00000000-0000-0000-0000-000000000012" force_deduplication="would not rename" +group "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000006" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000006" +group "exact-dupe-1" with namespace ID "admin" duplicates 1 others: id="00000000-0000-0000-0000-000000000007" force_deduplication="would not rename" +group "different-case-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000008" force_deduplication="would rename to different-case-dupe-1-00000000-0000-0000-0000-000000000008" +group "DIFFERENT-CASE-DUPE-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000009" force_deduplication="would not rename" +group "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000004" force_deduplication="would rename to exact-dupe-1-00000000-0000-0000-0000-000000000004" +group "exact-dupe-1" with namespace ID "root" duplicates 1 others: id="00000000-0000-0000-0000-000000000005" force_deduplication="would not rename" +end of group duplicates: +end of identity duplicate report, refer to https://developer.hashicorp.com/vault/docs/upgrading/identity-deduplication for resolution.: +` + + // Create a new errorResolver + r := newDuplicateReportingErrorResolver(log.NewNullLogger()) + + for i, pair := range entities { + // Create a fake UUID based on the index this makes sure sort order is + // preserved when eyeballing the expected report. + id := fmt.Sprintf("00000000-0000-0000-0000-%012d", i) + // Create a new entity with the pair + entity := &identity.Entity{ + ID: id, + Name: pair[1], + NamespaceID: pair[0], + } + + // Call ResolveEntities, assume existing is nil for now. In real life we + // should be passed the existing entity for the exact match dupes but we + // don't depend on that so it's fine to omit. + _ = r.ResolveEntities(context.Background(), nil, entity) + // Don't care about the actual error here since it would be ignored in + // case-sensitive mode anyway. + + // Also, since the data model is the same, pretend these are groups too + group := &identity.Group{ + ID: id, + Name: pair[1], + NamespaceID: pair[0], + } + _ = r.ResolveGroups(context.Background(), nil, group) + } + + // Load aliases second because that is realistic and yet we want to report on + // them first. + for i, pair := range aliases { + entity := &identity.Entity{ + ID: fmt.Sprintf("11111111-0000-0000-0000-%012d", i), + Name: pair[1] + "-entity", + NamespaceID: "root", + } + alias := &identity.Alias{ + ID: fmt.Sprintf("00000000-0000-0000-0000-%012d", i), + CanonicalID: entity.ID, + Name: pair[1], + MountAccessor: pair[0], + // Parse our hacky DSL to define some alias mounts as local + Local: strings.HasPrefix(pair[0], "local-"), + } + _ = r.ResolveAliases(context.Background(), entity, nil, alias) + } + + // "log" the report and check it matches expected report below. + var testLog identityTestWarnLogger + r.LogReport(&testLog) + + // Dump the raw report to make it easier to copy paste/read + t.Log("\n\n" + testLog.buf.String()) + + require.Equal(t, + strings.TrimSpace(expectReport), + strings.TrimSpace(testLog.buf.String()), + ) +} + +type identityTestWarnLogger struct { + buf bytes.Buffer +} + +func (l *identityTestWarnLogger) Warn(msg string, args ...interface{}) { + l.buf.WriteString(msg + ":") + if len(args)%2 != 0 { + panic("args must be key-value pairs") + } + for i := 0; i < len(args); i += 2 { + l.buf.WriteString(fmt.Sprintf(" %s=%q", args[i], args[i+1])) + } + l.buf.WriteString("\n") +} diff --git a/vault/identity_store_test.go b/vault/identity_store_test.go index cce4da413c..7ad41261ed 100644 --- a/vault/identity_store_test.go +++ b/vault/identity_store_test.go @@ -7,12 +7,16 @@ import ( "context" "fmt" "math/rand" + "regexp" + "slices" + "strconv" "strings" "testing" "time" "github.com/armon/go-metrics" "github.com/go-test/deep" + "github.com/hashicorp/go-hclog" uuid "github.com/hashicorp/go-uuid" credGithub "github.com/hashicorp/vault/builtin/credential/github" "github.com/hashicorp/vault/builtin/credential/userpass" @@ -1457,10 +1461,10 @@ func TestEntityStoreLoadingIsDeterministic(t *testing.T) { // probability that is unrealistic but ensures we have duplicates on every // test run with high probability and more than 1 duplicate often. for i := 0; i <= 100; i++ { - id := fmt.Sprintf("entity-%d", i) + name := fmt.Sprintf("entity-%d", i) alias := fmt.Sprintf("alias-%d", i) localAlias := fmt.Sprintf("localalias-%d", i) - e := makeEntityForPacker(t, id, c.identityStore.entityPacker) + e := makeEntityForPacker(t, name, c.identityStore.entityPacker) attachAlias(t, e, alias, upme) attachAlias(t, e, localAlias, localMe) err = TestHelperWriteToStoragePacker(ctx, c.identityStore.entityPacker, e.ID, e) @@ -1493,6 +1497,14 @@ func TestEntityStoreLoadingIsDeterministic(t *testing.T) { rnd = rand.Float64() dupeNum++ } + // See if we should add entity _name_ duplicates too (with no aliases) + rnd = rand.Float64() + for rnd < pDup { + e := makeEntityForPacker(t, name, c.identityStore.entityPacker) + err = TestHelperWriteToStoragePacker(ctx, c.identityStore.entityPacker, e.ID, e) + require.NoError(t, err) + rnd = rand.Float64() + } // One more edge case is that it's currently possible as of the time of // writing for a failure during entity invalidation to result in a permanent // "cached" entity in the local alias packer even though we do have the @@ -1511,23 +1523,27 @@ func TestEntityStoreLoadingIsDeterministic(t *testing.T) { // Create some groups for i := 0; i <= 100; i++ { - id := fmt.Sprintf("group-%d", i) - bucketKey := c.identityStore.groupPacker.BucketKey(id) + name := fmt.Sprintf("group-%d", i) // Add an alias to every other group alias := "" if i%2 == 0 { alias = fmt.Sprintf("groupalias-%d", i) } - e := makeGroupWithIDAndAlias(t, id, alias, bucketKey, upme) + e := makeGroupWithNameAndAlias(t, name, alias, c.identityStore.groupPacker, upme) err = TestHelperWriteToStoragePacker(ctx, c.identityStore.groupPacker, e.ID, e) require.NoError(t, err) } // Now add 10 groups with the same alias to ensure duplicates don't cause // non-deterministic behavior. for i := 0; i <= 10; i++ { - id := fmt.Sprintf("group-dup-%d", i) - bucketKey := c.identityStore.groupPacker.BucketKey(id) - e := makeGroupWithIDAndAlias(t, id, "groupalias-dup", bucketKey, upme) + name := fmt.Sprintf("group-dup-%d", i) + e := makeGroupWithNameAndAlias(t, name, "groupalias-dup", c.identityStore.groupPacker, upme) + err = TestHelperWriteToStoragePacker(ctx, c.identityStore.groupPacker, e.ID, e) + require.NoError(t, err) + } + // Add a second and third groups with duplicate names too. + for _, name := range []string{"group-0", "group-1", "group-1"} { + e := makeGroupWithNameAndAlias(t, name, "", c.identityStore.groupPacker, upme) err = TestHelperWriteToStoragePacker(ctx, c.identityStore.groupPacker, e.ID, e) require.NoError(t, err) } @@ -1539,14 +1555,14 @@ func TestEntityStoreLoadingIsDeterministic(t *testing.T) { // To test that this is deterministic we need to load from storage a bunch of // times and make sure we get the same result. For easier debugging we'll // build a list of human readable ids that we can compare. - lastIDs := []string{} + prevLoadedNames := []string{} for i := 0; i < 10; i++ { // Seal and unseal to reload the identity store require.NoError(t, c.Seal(rootToken)) require.True(t, c.Sealed()) for _, key := range sealKeys { unsealed, err := c.Unseal(key) - require.NoError(t, err) + require.NoError(t, err, "failed unseal on attempt %d", i) if unsealed { break } @@ -1554,7 +1570,7 @@ func TestEntityStoreLoadingIsDeterministic(t *testing.T) { require.False(t, c.Sealed()) // Identity store should be loaded now. Check it's contents. - loadedIDs := []string{} + loadedNames := []string{} tx := c.identityStore.db.Txn(false) @@ -1562,73 +1578,143 @@ func TestEntityStoreLoadingIsDeterministic(t *testing.T) { iter, err := tx.LowerBound(entitiesTable, "id", "") require.NoError(t, err) for item := iter.Next(); item != nil; item = iter.Next() { - // We already added "type" prefixes to the IDs when creating them so just - // append here. e := item.(*identity.Entity) - loadedIDs = append(loadedIDs, e.ID) + loadedNames = append(loadedNames, e.Name) for _, a := range e.Aliases { - loadedIDs = append(loadedIDs, a.ID) + loadedNames = append(loadedNames, a.Name) } } // This is a non-triviality check to make sure we actually loaded stuff and // are not just passing because of a bug in the test. - numLoaded := len(loadedIDs) + numLoaded := len(loadedNames) require.Greater(t, numLoaded, 300, "not enough entities and aliases loaded on attempt %d", i) + // Standalone alias query + iter, err = tx.LowerBound(entityAliasesTable, "id", "") + require.NoError(t, err) + for item := iter.Next(); item != nil; item = iter.Next() { + a := item.(*identity.Alias) + loadedNames = append(loadedNames, a.Name) + } + // Groups iter, err = tx.LowerBound(groupsTable, "id", "") require.NoError(t, err) for item := iter.Next(); item != nil; item = iter.Next() { g := item.(*identity.Group) - loadedIDs = append(loadedIDs, g.ID) + loadedNames = append(loadedNames, g.Name) if g.Alias != nil { - loadedIDs = append(loadedIDs, g.Alias.ID) + loadedNames = append(loadedNames, g.Alias.Name) } } // This is a non-triviality check to make sure we actually loaded stuff and // are not just passing because of a bug in the test. - groupsLoaded := len(loadedIDs) - numLoaded + groupsLoaded := len(loadedNames) - numLoaded require.Greater(t, groupsLoaded, 140, "not enough groups and aliases loaded on attempt %d", i) - entIdentityStoreDeterminismAssert(t, i, loadedIDs, lastIDs) + // note `lastIDs` argument is not needed any more but we can't change the + // signature without breaking enterprise. It's simpler to keep it unused for + // now until both parts of this merge. + entIdentityStoreDeterminismAssert(t, i, loadedNames, nil) if i > 0 { // Should be in the same order if we are deterministic since MemDB has strong ordering. - require.Equal(t, lastIDs, loadedIDs, "different result on attempt %d", i) + require.Equal(t, prevLoadedNames, loadedNames, "different result on attempt %d", i) } - lastIDs = loadedIDs + + prevLoadedNames = loadedNames } } -func makeGroupWithIDAndAlias(t *testing.T, id, alias, bucketKey string, me *MountEntry) *identity.Group { - g := &identity.Group{ - ID: id, - Name: id, - NamespaceID: namespace.RootNamespaceID, - BucketKey: bucketKey, - } - if alias != "" { - g.Alias = &identity.Alias{ - ID: id, - Name: alias, - CanonicalID: id, - MountType: me.Type, - MountAccessor: me.Accessor, - } - } - return g -} +// TestEntityStoreLoadingDuplicateReporting tests the reporting of different +// types of duplicates during unseal when in case-sensitive mode. +func TestEntityStoreLoadingDuplicateReporting(t *testing.T) { + logger := corehelpers.NewTestLogger(t) + ims, err := inmem.NewTransactionalInmemHA(nil, logger) + require.NoError(t, err) -func makeLocalAliasWithID(t *testing.T, id, entityID string, bucketKey string, me *MountEntry) *identity.LocalAliases { - return &identity.LocalAliases{ - Aliases: []*identity.Alias{ - { - ID: id, - Name: id, - CanonicalID: entityID, - MountType: me.Type, - MountAccessor: me.Accessor, - }, + cfg := &CoreConfig{ + Physical: ims, + HAPhysical: ims.(physical.HABackend), + Logger: logger, + BuiltinRegistry: corehelpers.NewMockBuiltinRegistry(), + CredentialBackends: map[string]logical.Factory{ + "userpass": userpass.Factory, }, } + + c, sealKeys, rootToken := TestCoreUnsealedWithConfig(t, cfg) + + // Inject values into storage + upme, err := TestUserpassMount(c, false) + require.NoError(t, err) + localMe, err := TestUserpassMount(c, true) + require.NoError(t, err) + + ctx := namespace.RootContext(nil) + + identityCreateCaseDuplicates(t, ctx, c, upme, localMe) + + entIdentityStoreDuplicateReportTestSetup(t, ctx, c, rootToken) + + // Storage is now primed for the test. + + // Seal and unseal to reload the identity store + require.NoError(t, c.Seal(rootToken)) + require.True(t, c.Sealed()) + + // Setup a logger we can use to capture unseal logs + var unsealLogs []string + unsealLogger := &logFn{ + fn: func(msg string, args []interface{}) { + pairs := make([]string, 0, len(args)/2) + for pair := range slices.Chunk(args, 2) { + // Yes this will panic if we didn't log an even number of args but thats + // OK because that's a bug! + pairs = append(pairs, fmt.Sprintf("%s=%s", pair[0], pair[1])) + } + unsealLogs = append(unsealLogs, fmt.Sprintf("%s: %s", msg, strings.Join(pairs, " "))) + }, + } + logger.RegisterSink(unsealLogger) + + for _, key := range sealKeys { + unsealed, err := c.Unseal(key) + require.NoError(t, err) + if unsealed { + break + } + } + require.False(t, c.Sealed()) + logger.DeregisterSink(unsealLogger) + + // Identity store should be loaded now. Check it's contents. + + // We don't expect any actual behavior change just logs reporting duplicates. + // We could assert the current "expected" behavior but it's actually broken in + // many of these cases and seems strange to encode in a test that we want + // broken behavior! + numDupes := make(map[string]int) + duplicateCountRe := regexp.MustCompile(`(\d+) (different-case( local)? entity alias|entity|group) duplicates found`) + for _, log := range unsealLogs { + if matches := duplicateCountRe.FindStringSubmatch(log); len(matches) >= 3 { + num, _ := strconv.Atoi(matches[1]) + numDupes[matches[2]] = num + } + } + t.Logf("numDupes: %v", numDupes) + wantAliases, wantLocalAliases, wantEntities, wantGroups := identityStoreDuplicateReportTestWantDuplicateCounts() + require.Equal(t, wantLocalAliases, numDupes["different-case local entity alias"]) + require.Equal(t, wantAliases, numDupes["different-case entity alias"]) + require.Equal(t, wantEntities, numDupes["entity"]) + require.Equal(t, wantGroups, numDupes["group"]) +} + +type logFn struct { + fn func(msg string, args []interface{}) +} + +// Accept implements hclog.SinkAdapter +func (f *logFn) Accept(name string, level hclog.Level, msg string, args ...interface{}) { + f.fn(msg, args) } diff --git a/vault/identity_store_test_stubs_oss.go b/vault/identity_store_test_stubs_oss.go index e3a5703885..821b62428c 100644 --- a/vault/identity_store_test_stubs_oss.go +++ b/vault/identity_store_test_stubs_oss.go @@ -12,10 +12,25 @@ import ( //go:generate go run github.com/hashicorp/vault/tools/stubmaker -func entIdentityStoreDeterminismTestSetup(t *testing.T, ctx context.Context, c *Core, upme, localme *MountEntry) { +func entIdentityStoreDeterminismTestSetup(t *testing.T, ctx context.Context, c *Core, me, localme *MountEntry) { // no op } func entIdentityStoreDeterminismAssert(t *testing.T, i int, loadedIDs, lastIDs []string) { // no op } + +func entIdentityStoreDuplicateReportTestSetup(t *testing.T, ctx context.Context, c *Core, rootToken string) { + // no op +} + +func identityStoreDuplicateReportTestWantDuplicateCounts() (int, int, int, int) { + // Note that the second count is for local aliases. CE Vault doesn't really + // distinguish between local and non-local aliases because it doesn't have any + // support for Performance Replication. But it's possible in code at least to + // set the local flag on a mount or alias during creation so we might as well + // test it behaves as expected in the CE code. It's maybe just about possible + // that this could happen in real life too because of a downgrade from + // Enterprise. + return 1, 1, 1, 1 +} diff --git a/vault/identity_store_util.go b/vault/identity_store_util.go index 845b0672d1..602f34016e 100644 --- a/vault/identity_store_util.go +++ b/vault/identity_store_util.go @@ -23,6 +23,7 @@ import ( "github.com/hashicorp/vault/helper/storagepacker" "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/logical" + "github.com/stretchr/testify/require" "google.golang.org/protobuf/types/known/anypb" "google.golang.org/protobuf/types/known/timestamppb" ) @@ -83,9 +84,19 @@ func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error { return err } + // Also reset the conflict resolver so that we report potential duplicates to + // be resolved before it's safe to return to case-insensitive mode. + reporterResolver := newDuplicateReportingErrorResolver(c.identityStore.logger) + c.identityStore.conflictResolver = reporterResolver + // Attempt to load identity artifacts once more after memdb is reset to // accept case sensitive names - return loadFunc(ctx) + err = loadFunc(ctx) + + // Log reported duplicates if any found whether or not we end up erroring. + reporterResolver.LogReport(c.identityStore.logger) + + return err } func (i *IdentityStore) sanitizeName(name string) string { @@ -623,8 +634,8 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e default: // Though this is technically a conflict that should be resolved by the // ConflictResolver implementation, the behavior here is a bit nuanced. - // Rather than introduce a behavior change, handle this case directly as - // before by merging. + // Rather than introduce a behavior change, we handle this case directly + // as before by merging. i.logger.Warn("alias is already tied to a different entity; these entities are being merged", "alias_id", alias.ID, "other_entity_id", aliasByFactors.CanonicalID, @@ -644,10 +655,34 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e return nil } - if strutil.StrListContains(aliasFactors, i.sanitizeName(alias.Name)+alias.MountAccessor) { - if err := i.conflictResolver.ResolveAliases(ctx, entity, aliasByFactors, alias); err != nil && !i.disableLowerCasedNames { - return err - } + // This is subtle. We want to call `ResolveAliases` so that the resolver can + // get full insight into all the aliases loaded and generate useful reports + // about duplicates. However, we don't want to actually change the error + // handling behavior from before which would only return an error in a very + // specific case (when the alias being added is a duplicate of one for the + // same entity and we are not in case-sensitive mode). So we call the method + // here unconditionally, but then only handle the resultant error in the + // specific case we care about. Note that we choose not to call it `err` to + // avoid it being left non-nil in some cases and tripping up later error + // handling code, and to signal something different is happening here. Note + // that we explicitly _want_ this to be here an not before we merge + // duplicates above, because duplicates that have always merged are not a + // problem to the user and are already logged. We care about different-case + // duplicates that are not being considered duplicates right now because we + // are in case-sensitive mode so we can report these to the operator ahead + // of them disabling case-sensitive mode. + conflictErr := i.conflictResolver.ResolveAliases(ctx, entity, aliasByFactors, alias) + + // This appears to be accounting for any duplicate aliases for the same + // Entity. In that case we would have skipped over the merge above in the + // `aliasByFactors.CanonicalID == entity.ID` case and made it here. Now we + // are here, duplicates are reported and may cause an insert error but only + // if we are in default case-insensitive mode. Once we are in case-sensitive + // mode we'll happily ignore duplicates of any case! This doesn't seem + // especially desirable to me, but we'd rather not change behavior for now. + if strutil.StrListContains(aliasFactors, i.sanitizeName(alias.Name)+alias.MountAccessor) && + conflictErr != nil && !i.disableLowerCasedNames { + return conflictErr } // Insert or update alias in MemDB using the transaction created above @@ -2636,24 +2671,113 @@ func (i *IdentityStore) countEntitiesByMountAccessor(ctx context.Context) (map[s return byMountAccessor, nil } -func makeEntityForPacker(_t *testing.T, id string, p *storagepacker.StoragePacker) *identity.Entity { +func makeEntityForPacker(t *testing.T, name string, p *storagepacker.StoragePacker) *identity.Entity { + t.Helper() + return makeEntityForPackerWithNamespace(t, namespace.RootNamespaceID, name, p) +} + +func makeEntityForPackerWithNamespace(t *testing.T, namespaceID, name string, p *storagepacker.StoragePacker) *identity.Entity { + t.Helper() + id, err := uuid.GenerateUUID() + require.NoError(t, err) return &identity.Entity{ ID: id, - Name: id, - NamespaceID: namespace.RootNamespaceID, + Name: name, + NamespaceID: namespaceID, BucketKey: p.BucketKey(id), } } func attachAlias(t *testing.T, e *identity.Entity, name string, me *MountEntry) *identity.Alias { t.Helper() + id, err := uuid.GenerateUUID() + require.NoError(t, err) + if e.NamespaceID != me.NamespaceID { + panic("mount and entity in different namespaces") + } a := &identity.Alias{ - ID: name, + ID: id, Name: name, + NamespaceID: me.NamespaceID, CanonicalID: e.ID, MountType: me.Type, MountAccessor: me.Accessor, + Local: me.Local, } e.UpsertAlias(a) return a } + +func identityCreateCaseDuplicates(t *testing.T, ctx context.Context, c *Core, upme, localme *MountEntry) { + t.Helper() + + if upme.NamespaceID != localme.NamespaceID { + panic("both replicated and local auth mounts must be in the same namespace") + } + + // Create entities with both case-sensitive and case-insensitive duplicate + // suffixes. + for i, suffix := range []string{"-case", "-case", "-cAsE"} { + // Entity duplicated by name + e := makeEntityForPackerWithNamespace(t, upme.NamespaceID, "entity"+suffix, c.identityStore.entityPacker) + err := TestHelperWriteToStoragePacker(ctx, c.identityStore.entityPacker, e.ID, e) + require.NoError(t, err) + + // Entity that isn't a dupe itself but has duplicated aliases + e2 := makeEntityForPackerWithNamespace(t, upme.NamespaceID, fmt.Sprintf("entity-%d", i), c.identityStore.entityPacker) + // Add local and non-local aliases for this entity (which will also be + // duplicated) + attachAlias(t, e2, "alias"+suffix, upme) + attachAlias(t, e2, "local-alias"+suffix, localme) + err = TestHelperWriteToStoragePacker(ctx, c.identityStore.entityPacker, e2.ID, e2) + require.NoError(t, err) + + // Group duplicated by name + g := makeGroupWithNameAndAlias(t, "group"+suffix, "", c.identityStore.groupPacker, upme) + err = TestHelperWriteToStoragePacker(ctx, c.identityStore.groupPacker, g.ID, g) + require.NoError(t, err) + } +} + +func makeGroupWithNameAndAlias(t *testing.T, name, alias string, p *storagepacker.StoragePacker, me *MountEntry) *identity.Group { + t.Helper() + id, err := uuid.GenerateUUID() + require.NoError(t, err) + id2, err := uuid.GenerateUUID() + require.NoError(t, err) + g := &identity.Group{ + ID: id, + Name: name, + NamespaceID: me.NamespaceID, + BucketKey: p.BucketKey(id), + } + if alias != "" { + g.Alias = &identity.Alias{ + ID: id2, + Name: alias, + CanonicalID: id, + MountType: me.Type, + MountAccessor: me.Accessor, + NamespaceID: me.NamespaceID, + } + } + return g +} + +func makeLocalAliasWithName(t *testing.T, name, entityID string, bucketKey string, me *MountEntry) *identity.LocalAliases { + t.Helper() + id, err := uuid.GenerateUUID() + require.NoError(t, err) + return &identity.LocalAliases{ + Aliases: []*identity.Alias{ + { + ID: id, + Name: name, + CanonicalID: entityID, + MountType: me.Type, + MountAccessor: me.Accessor, + NamespaceID: me.NamespaceID, + }, + }, + } +} diff --git a/vault/testing.go b/vault/testing.go index bd5c6d0fca..30575029cc 100644 --- a/vault/testing.go +++ b/vault/testing.go @@ -2225,6 +2225,10 @@ var ( ) func TestUserpassMount(c *Core, local bool) (*MountEntry, error) { + return TestUserpassMountContext(namespace.RootContext(nil), c, local) +} + +func TestUserpassMountContext(ctx context.Context, c *Core, local bool) (*MountEntry, error) { name := "userpass" if local { name += "-local" @@ -2234,10 +2238,12 @@ func TestUserpassMount(c *Core, local bool) (*MountEntry, error) { Path: name + "/", Type: "userpass", Description: name, - Accessor: name, - Local: local, + // Don't specify an accessor so we use a random one otherwise we will cause + // horrible issues when we try to create a new mount on a different + // namespace but they have the same accessor! + Local: local, } - if err := c.enableCredential(namespace.RootContext(nil), userpassMe); err != nil { + if err := c.enableCredential(ctx, userpassMe); err != nil { return nil, err } return userpassMe, nil