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

Remove caseSensitivityKey invalidation #20965

Merged
merged 13 commits into from
Jul 14, 2023
3 changes: 3 additions & 0 deletions changelog/20965.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
```release-note:bug
identity: Remove caseSensitivityKey to prevent errors while loading groups which could result in missing groups in memDB when duplicates are found.
```
4 changes: 0 additions & 4 deletions vault/core.go
Original file line number Diff line number Diff line change
Expand Up @@ -593,10 +593,6 @@ type Core struct {
// active, or give up active as soon as it gets it
neverBecomeActive *uint32

// loadCaseSensitiveIdentityStore enforces the loading of identity store
// artifacts in a case sensitive manner. To be used only in testing.
loadCaseSensitiveIdentityStore bool

// clusterListener starts up and manages connections on the cluster ports
clusterListener *atomic.Value

Expand Down
66 changes: 22 additions & 44 deletions vault/identity_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -576,14 +576,31 @@ func (i *IdentityStore) initialize(ctx context.Context, req *logical.Initializat
return err
}

entry, err := logical.StorageEntryJSON(caseSensitivityKey, &casesensitivity{
DisableLowerCasedNames: i.disableLowerCasedNames,
})
// if the storage entry for caseSensitivityKey exists, remove it
storageEntry, err := i.view.Get(ctx, caseSensitivityKey)
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need a Get, you can just Delete - it won't return an error if the thing is already missing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Although now that I think about it, let's keep this, and add a log line where we record the value it had at time of deletion. Could be useful in debugging at some point.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added debug line to get caseSensitivityKey value at the time of deletion

if err != nil {
return err
i.logger.Error("could not get storage entry for case sensitivity key", "error", err)
return nil
}

if storageEntry != nil {
var setting casesensitivity
err := storageEntry.DecodeJSON(&setting)
switch err {
case nil:
i.logger.Debug("removing storage entry for case sensitivity key", "value", setting.DisableLowerCasedNames)
default:
i.logger.Error("failed to decode case sensitivity key, removing its storage entry anyway", "error", err)
}

err = i.view.Delete(ctx, caseSensitivityKey)
if err != nil {
i.logger.Error("could not delete storage entry for case sensitivity key", "error", err)
return nil
}
}

return i.view.Put(ctx, entry)
return nil
akshya96 marked this conversation as resolved.
Show resolved Hide resolved
}

// Invalidate is a callback wherein the backend is informed that the value at
Expand All @@ -597,45 +614,6 @@ func (i *IdentityStore) Invalidate(ctx context.Context, key string) {
defer i.lock.Unlock()

switch {
case key == caseSensitivityKey:
entry, err := i.view.Get(ctx, caseSensitivityKey)
if err != nil {
i.logger.Error("failed to read case sensitivity setting during invalidation", "error", err)
return
}
if entry == nil {
return
}

var setting casesensitivity
if err := entry.DecodeJSON(&setting); err != nil {
i.logger.Error("failed to decode case sensitivity setting during invalidation", "error", err)
return
}

// Fast return if the setting is the same
if i.disableLowerCasedNames == setting.DisableLowerCasedNames {
return
}

// If the setting is different, reset memdb and reload all the artifacts
i.disableLowerCasedNames = setting.DisableLowerCasedNames
if err := i.resetDB(ctx); err != nil {
i.logger.Error("failed to reset memdb during invalidation", "error", err)
return
}
if err := i.loadEntities(ctx); err != nil {
i.logger.Error("failed to load entities during invalidation", "error", err)
return
}
if err := i.loadGroups(ctx); err != nil {
i.logger.Error("failed to load groups during invalidation", "error", err)
return
}
if err := i.loadOIDCClients(ctx); err != nil {
i.logger.Error("failed to load OIDC clients during invalidation", "error", err)
return
}
// Check if the key is a storage entry key for an entity bucket
case strings.HasPrefix(key, storagepacker.StoragePackerBucketsPrefix):
// Create a MemDB transaction
Expand Down
56 changes: 56 additions & 0 deletions vault/identity_store_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -857,3 +857,59 @@ func TestIdentityStore_UpdateAliasMetadataPerAccessor(t *testing.T) {
t.Fatalf("wrong alias index changed. Expected 1, got %d", i)
}
}

// TestIdentityStore_DeleteCaseSensitivityKey tests that
// casesensitivity key gets removed from storage if it exists upon
// initializing identity store.
func TestIdentityStore_DeleteCaseSensitivityKey(t *testing.T) {
c, unsealKey, root := TestCoreUnsealed(t)
ctx := context.Background()

// add caseSensitivityKey to storage
entry, err := logical.StorageEntryJSON(caseSensitivityKey, &casesensitivity{
DisableLowerCasedNames: true,
})
if err != nil {
t.Fatal(err)
}
err = c.identityStore.view.Put(ctx, entry)
if err != nil {
t.Fatal(err)
}

// check if the value is stored in storage
storageEntry, err := c.identityStore.view.Get(ctx, caseSensitivityKey)
if err != nil {
t.Fatal(err)
}

if storageEntry == nil {
t.Fatalf("bad: expected a non-nil entry for casesensitivity key")
}

// Seal and unseal to trigger identityStore initialize
if err = c.Seal(root); err != nil {
t.Fatal(err)
}

var unsealed bool
for i := 0; i < len(unsealKey); i++ {
unsealed, err = c.Unseal(unsealKey[i])
if err != nil {
t.Fatal(err)
}
}
if !unsealed {
t.Fatal("still sealed")
}

// check if caseSensitivityKey exists after initialize
storageEntry, err = c.identityStore.view.Get(ctx, caseSensitivityKey)
if err != nil {
t.Fatal(err)
}

if storageEntry != nil {
t.Fatalf("bad: expected no entry for casesensitivity key")
}
}
22 changes: 8 additions & 14 deletions vault/identity_store_util.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ var (
tmpSuffix = ".tmp"
)

func (c *Core) SetLoadCaseSensitiveIdentityStore(caseSensitive bool) {
c.loadCaseSensitiveIdentityStore = caseSensitive
}

func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error {
if c.identityStore == nil {
c.logger.Warn("identity store is not setup, skipping loading")
Expand All @@ -58,16 +54,14 @@ func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error {
return nil
}

if !c.loadCaseSensitiveIdentityStore {
// Load everything when memdb is set to operate on lower cased names
err := loadFunc(ctx)
switch {
case err == nil:
// If it succeeds, all is well
return nil
case !errwrap.Contains(err, errDuplicateIdentityName.Error()):
return err
}
// Load everything when memdb is set to operate on lower cased names
err := loadFunc(ctx)
switch {
case err == nil:
// If it succeeds, all is well
return nil
case !errwrap.Contains(err, errDuplicateIdentityName.Error()):
return err
}

c.identityStore.logger.Warn("enabling case sensitive identity names")
Expand Down