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.

Sign up for GitHub

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

Persist merged entities only on the primary #6075

Merged
merged 15 commits into from
Feb 8, 2019
Merged

Persist merged entities only on the primary #6075

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
147743b
Merge entities during unseal only on the primary
vishalnayak Jan 18, 2019
b12a558
Add another guard check
vishalnayak Jan 18, 2019
dee8f9c
Add perf standby to the check
vishalnayak Feb 1, 2019
162d093
Make primary to not differ from case-insensitivity status w.r.t secon…
vishalnayak Feb 4, 2019
da27715
Ensure mutual exclusivity between loading and invalidations
vishalnayak Feb 4, 2019
3fb55b6
Both primary and secondaries won't persist during startup and invalid…
vishalnayak Feb 4, 2019
4df3d3e
Allow primary to persist when loading case sensitively
vishalnayak Feb 4, 2019
df84739
Using core.perfStandby
vishalnayak Feb 5, 2019
5020513
Add a tweak in core for testing
vishalnayak Feb 7, 2019
5ac6370
Address review feedback
vishalnayak Feb 7, 2019
f2d0e83
update memdb but not storage in secondaries
vishalnayak Feb 7, 2019
41a47c2
Wire all the things directly do mergeEntity
vishalnayak Feb 7, 2019
d862f84
Fix persist behavior
vishalnayak Feb 7, 2019
4ac25c6
Address review feedback
vishalnayak Feb 8, 2019
ce9d094
Merge branch 'master' into entity-loading
vishalnayak Feb 8, 2019
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
4 changes: 4 additions & 0 deletions vault/core.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -416,6 +416,10 @@ type Core struct {
// Can be toggled atomically to cause the core to never try to become
// 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
}

// CoreConfig is used to parameterize a core
Expand Down
41 changes: 23 additions & 18 deletions vault/identity_store_entities.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"github.com/golang/protobuf/ptypes"
"github.com/hashicorp/errwrap"
memdb "github.com/hashicorp/go-memdb"
"github.com/hashicorp/vault/helper/consts"
"github.com/hashicorp/vault/helper/identity"
"github.com/hashicorp/vault/helper/namespace"
"github.com/hashicorp/vault/helper/storagepacker"
Expand Down Expand Up @@ -155,7 +156,7 @@ func (i *IdentityStore) pathEntityMergeID() framework.OperationFunc {
return nil, err
}

userErr, intErr := i.mergeEntity(ctx, txn, toEntity, fromEntityIDs, force, true, false)
userErr, intErr := i.mergeEntity(ctx, txn, toEntity, fromEntityIDs, force, true, false, true)
if userErr != nil {
return logical.ErrorResponse(userErr.Error()), nil
}
Expand Down Expand Up @@ -604,7 +605,7 @@ func (i *IdentityStore) handlePathEntityListCommon(ctx context.Context, req *log
return logical.ListResponseWithInfo(keys, entityInfo), nil
}

func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntity *identity.Entity, fromEntityIDs []string, force, grabLock, mergePolicies bool) (error, error) {
func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntity *identity.Entity, fromEntityIDs []string, force, grabLock, mergePolicies, persist bool) (error, error) {
if grabLock {
i.lock.Lock()
defer i.lock.Unlock()
Expand Down Expand Up @@ -704,10 +705,12 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit
return nil, err
}

// Delete the entity which we are merging from in storage
err = i.entityPacker.DeleteItem(fromEntity.ID)
if err != nil {
return nil, err
if persist && !(i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby) {
// Delete the entity which we are merging from in storage
err = i.entityPacker.DeleteItem(fromEntity.ID)
if err != nil {
return nil, err
}
}
}

Expand All @@ -717,19 +720,21 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit
return nil, err
}

// Persist the entity which we are merging to
toEntityAsAny, err := ptypes.MarshalAny(toEntity)
if err != nil {
return nil, err
}
item := &storagepacker.Item{
ID: toEntity.ID,
Message: toEntityAsAny,
}
if persist && !(i.core.ReplicationState().HasState(consts.ReplicationPerformanceSecondary) || i.core.perfStandby) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor nit, can you pull the replication state check into a var at the top of the function rather than have it twice?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

// Persist the entity which we are merging to
toEntityAsAny, err := ptypes.MarshalAny(toEntity)
if err != nil {
return nil, err
}
item := &storagepacker.Item{
ID: toEntity.ID,
Message: toEntityAsAny,
}

err = i.entityPacker.PutItem(item)
if err != nil {
return nil, err
err = i.entityPacker.PutItem(item)
if err != nil {
return nil, err
}
}

return nil, nil
Expand Down
152 changes: 113 additions & 39 deletions vault/identity_store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,98 @@ import (
"github.com/hashicorp/vault/logical"
)

func TestIdentityStore_UnsealingWhenConflictingAliasNames(t *testing.T) {
err := AddTestCredentialBackend("github", credGithub.Factory)
if err != nil {
t.Fatalf("err: %s", err)
}

c, unsealKey, root := TestCoreUnsealed(t)

meGH := &MountEntry{
Table: credentialTableType,
Path: "github/",
Type: "github",
Description: "github auth",
}

err = c.enableCredential(namespace.RootContext(nil), meGH)
if err != nil {
t.Fatal(err)
}

alias := &identity.Alias{
ID: "alias1",
CanonicalID: "entity1",
MountType: "github",
MountAccessor: meGH.Accessor,
Name: "githubuser",
}
entity := &identity.Entity{
ID: "entity1",
Name: "name1",
Policies: []string{"foo", "bar"},
Aliases: []*identity.Alias{
alias,
},
NamespaceID: namespace.RootNamespaceID,
}
entity.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity.ID)

err = c.identityStore.upsertEntity(namespace.RootContext(nil), entity, nil, true)
if err != nil {
t.Fatal(err)
}

alias2 := &identity.Alias{
ID: "alias2",
CanonicalID: "entity2",
MountType: "github",
MountAccessor: meGH.Accessor,
Name: "GITHUBUSER",
}
entity2 := &identity.Entity{
ID: "entity2",
Name: "name2",
Policies: []string{"foo", "bar"},
Aliases: []*identity.Alias{
alias2,
},
NamespaceID: namespace.RootNamespaceID,
}
entity2.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity2.ID)

// Persist the second entity directly without the regular flow. This will skip
// merging of these enties.
entity2Any, err := ptypes.MarshalAny(entity2)
if err != nil {
t.Fatal(err)
}
item := &storagepacker.Item{
ID: entity2.ID,
Message: entity2Any,
}
if err = c.identityStore.entityPacker.PutItem(item); err != nil {
t.Fatal(err)
}

// Seal and ensure that unseal works
if err = c.Seal(root); err != nil {
t.Fatal(err)
}

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

func TestIdentityStore_EntityIDPassthrough(t *testing.T) {
// Enable GitHub auth and initialize
ctx := namespace.RootContext(nil)
Expand Down Expand Up @@ -381,7 +473,7 @@ func TestIdentityStore_MergeConflictingAliases(t *testing.T) {
t.Fatalf("err: %s", err)
}

c, unsealKey, root := TestCoreUnsealed(t)
c, _, _ := TestCoreUnsealed(t)

meGH := &MountEntry{
Table: credentialTableType,
Expand Down Expand Up @@ -409,54 +501,36 @@ func TestIdentityStore_MergeConflictingAliases(t *testing.T) {
Aliases: []*identity.Alias{
alias,
},
NamespaceID: namespace.RootNamespaceID,
}
entity.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity.ID)
// Now add the alias to two entities, skipping all existing checking by
// writing directly
entityAny, err := ptypes.MarshalAny(entity)
err = c.identityStore.upsertEntity(namespace.RootContext(nil), entity, nil, true)
if err != nil {
t.Fatal(err)
}
item := &storagepacker.Item{
ID: entity.ID,
Message: entityAny,
}
if err = c.identityStore.entityPacker.PutItem(item); err != nil {
t.Fatal(err)
}

entity.ID = "entity2"
entity.Name = "name2"
entity.Policies = []string{"bar", "baz"}
alias.ID = "alias2"
alias.CanonicalID = "entity2"
entity.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity.ID)
entityAny, err = ptypes.MarshalAny(entity)
if err != nil {
t.Fatal(err)
}
item = &storagepacker.Item{
ID: entity.ID,
Message: entityAny,
alias2 := &identity.Alias{
ID: "alias2",
CanonicalID: "entity2",
MountType: "github",
MountAccessor: meGH.Accessor,
Name: "githubuser",
}
if err = c.identityStore.entityPacker.PutItem(item); err != nil {
t.Fatal(err)
entity2 := &identity.Entity{
ID: "entity2",
Name: "name2",
Policies: []string{"bar", "baz"},
Aliases: []*identity.Alias{
alias2,
},
NamespaceID: namespace.RootNamespaceID,
}

// Seal and unseal. If things are broken, we will now fail to unseal.
if err = c.Seal(root); err != nil {
t.Fatal(err)
}
entity2.BucketKeyHash = c.identityStore.entityPacker.BucketKeyHashByItemID(entity2.ID)

var unsealed bool
for i := 0; i < 3; i++ {
unsealed, err = c.Unseal(unsealKey[i])
if err != nil {
t.Fatal(err)
}
}
if !unsealed {
t.Fatal("still sealed")
err = c.identityStore.upsertEntity(namespace.RootContext(nil), entity2, nil, true)
if err != nil {
t.Fatal(err)
}

newEntity, err := c.identityStore.CreateOrFetchEntity(namespace.RootContext(nil), &logical.Alias{
Expand Down
31 changes: 19 additions & 12 deletions vault/identity_store_util.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,10 @@ var (
errDuplicateIdentityName = errors.New("duplicate identity name")
)

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 @@ -38,14 +42,16 @@ func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error {
return c.identityStore.loadGroups(ctx)
}

// 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 err != nil && !errwrap.Contains(err, errDuplicateIdentityName.Error()):
return err
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 err != nil && !errwrap.Contains(err, errDuplicateIdentityName.Error()):
return err
}
}

c.identityStore.logger.Warn("enabling case sensitive identity names")
Expand All @@ -56,8 +62,7 @@ func (c *Core) loadIdentityStoreArtifacts(ctx context.Context) error {
// Swap the memdb instance by the one which operates on case sensitive
// names, hence obviating the need to unload anything that's already
// loaded.
err = c.identityStore.resetDB(ctx)
if err != nil {
if err := c.identityStore.resetDB(ctx); err != nil {
return err
}

Expand Down Expand Up @@ -334,21 +339,23 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e
fallthrough
default:
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, "entity_aliases", entity.Aliases, "alias_by_factors", aliasByFactors)
respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true)

respErr, intErr := i.mergeEntity(ctx, txn, entity, []string{aliasByFactors.CanonicalID}, true, false, true, persist)
switch {
case respErr != nil:
return respErr
case intErr != nil:
return intErr
}

// The entity and aliases will be loaded into memdb and persisted
// as a result of the merge so we are done here
return nil
}

if strutil.StrListContains(aliasFactors, i.sanitizeName(alias.Name)+alias.MountAccessor) {
i.logger.Warn(errDuplicateIdentityName.Error(), "alias_name", alias.Name, "mount_accessor", alias.MountAccessor, "entity_name", entity.Name, "action", "delete one of the duplicate aliases")
if !i.disableLowerCasedNames {
if !persist && !i.disableLowerCasedNames {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be an OR, not AND?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should be an AND.

Copy link
Member

Choose a reason for hiding this comment

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

OK, so this will then cause memdb updates in this case, whereas before we would not do memdb updates and would simply return the error. Just making sure that's the correct behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for asking! The reason behind this change is just that disableLowerCasedNames error is captured only in the startup flow and no where else, and it only makes sense to get this error in that flow. But this function is not restricted to the login flow alone. So adding the guard of persist being set to false ensures that regular operations doesn't result in this error. This is not related to the core fix in this PR, and is something that I happened to notice.

return errDuplicateIdentityName
}
}
Expand Down