Case insensitive identity names #5404

vishalnayak · 2018-09-26T17:22:28Z

No description provided.

vishalnayak · 2018-09-27T21:21:04Z

This is ready for review. Did some upgrade testing as well and it all seems to work as intended.

tyrannosaurus-becks

This looks excellent, nice work @vishalnayak .

tyrannosaurus-becks · 2018-09-27T21:46:26Z

vault/identity_store_aliases.go

@@ -203,7 +200,7 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc {
 			// If we can look up by the given canonical ID, see if this is a
 			// transfer; otherwise if we found no previous entity but we find one
 			// here, use it.
-			canonicalEntity, err := i.MemDBEntityByID(canonicalID, true)
+			canonicalEntity, err := i.MemDBEntityByID(canonicalID, false)


I'm curious why this was flipped to false.

It was something I was trying while investigating an issue and had forgot to flip back. You pointed out a potential bug. Thanks! Fixed it.

kalafut · 2018-09-28T04:25:34Z

vault/identity_store_util.go

+				return err
+			}
+			if groupByName != nil && !i.core.disableCaseInsensitiveIdentityNames {
+				return fmt.Errorf(`Duplicate group names %q and %q.


Remove capitalization.

kalafut · 2018-09-28T04:25:41Z

vault/identity_store_util.go

+					return nil
+				}
+				if entityByName != nil && !i.core.disableCaseInsensitiveIdentityNames {
+					return fmt.Errorf(`Duplicate entity names %q and %q.


Remove capitalization.

kalafut · 2018-09-28T04:25:53Z

vault/identity_store_util.go

@@ -244,11 +277,20 @@ func (i *IdentityStore) upsertEntityInTxn(ctx context.Context, txn *memdb.Txn, e
 			return nil
 		}

+		if strutil.StrListContains(aliasFactors, i.sanitizeName(alias.Name)+alias.MountAccessor) && !i.core.disableCaseInsensitiveIdentityNames {
+			return fmt.Errorf(`Duplicate alias factors name=%q mount_accessor=%q for entity name %q.


Remove capitalization.

kalafut · 2018-09-28T04:28:57Z

vault/identity_store_group_aliases_test.go

+	testAliasName := "testAliasName"
+
+	// Create a case sensitive alias name
+	// Create a case sensitive alias name


vault/identity_store_util.go

kalafut · 2018-09-28T04:34:07Z

Just some minor observations, otherwise it looks good!

calvn · 2018-09-28T19:08:41Z

vault/identity_store_group_aliases_test.go

+	if err != nil || (resp != nil && resp.IsError()) {
+		t.Fatalf("bad: err:%v\nresp: %#v", err, resp)
+	}
+}


Perhaps test for actual vs expected value once more after the group alias has been updated.

Done, for both entity and group alias tests.

briankassouf · 2018-10-01T17:18:44Z

vault/identity_store.go

@@ -455,6 +473,7 @@ func (i *IdentityStore) CreateOrFetchEntity(ctx context.Context, alias *logical.
 	newAlias := &identity.Alias{
 		CanonicalID:   entity.ID,
 		Name:          alias.Name,


Can we sanitize the name here?

Nope. We don't need to sanitize here. It is done in the utility that inserts item into memdb.

briankassouf · 2018-10-01T17:20:14Z

vault/identity_store_aliases.go

@@ -138,6 +138,7 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc {
 			}
 		} else {
 			alias.Name = aliasName


Same, should we sanitize this here?

Same here. We don't need to sanitize here.

briankassouf · 2018-10-01T17:20:56Z

vault/identity_store_entities.go

@@ -214,6 +214,8 @@ func (i *IdentityStore) handleEntityUpdateCommon() framework.OperationFunc {

 		if entityName != "" {
 			entity.Name = entityName


Same, should we sanitize here?

briankassouf · 2018-10-01T17:21:29Z

vault/identity_store_groups.go

@@ -225,6 +225,7 @@ func (i *IdentityStore) handleGroupUpdateCommon(ctx context.Context, req *logica
 			return logical.ErrorResponse("group name is already in use"), nil
 		}
 		group.Name = groupName


Same, should we sanitize here?

briankassouf · 2018-10-01T17:22:25Z

vault/identity_store_util.go

+identity names are treated case insensitively unless
+'disable_case_insensitive_identity_names' config is set`,
+					group.NameRaw, groupByName.NameRaw)
+			}


This is likely to happen during upgrade right? Would this halt startup?

This worries me a bit, I wonder if there is a better escape hatch we can offer. It seems to me it's likely that many existing installations will hit this issue and need to disable case insensitivity, which defeats the purpose of making it case sensitive.

Yes this would happen during upgrade. The goal was to switch over all identity names to be case insensitive for good. It would still work seemlessly for those who are using case sensitive names. The conflict would only arise if there are duplicate names with different casings for entities, groups and/or respective aliases. In that case, they will have to set this flag to continue operating as they would. Or, they could set this flag, remove the duplicate items, and reset the flag to get along with the regular flow.

This should get a mention in the upgrade notes.

I think if disable is set, we should detect and warn. That way people can set the flag, start up, see if anything is in conflict, take appropriate action.

There is no flag now. So, the above discussion is not required.

calvn · 2018-10-04T19:36:44Z

vault/identity_store_util.go

+		// If it succeeds, all is well
+		return nil
+	case err != nil && !errwrap.ContainsType(err, errDuplicateIdentityName):
+		// Return all errors except one


What does this comment mean, are there multiple errors that we expect?

It meant to return all the errors expect when error contains errDuplicateIdentityName.

calvn · 2018-10-04T19:38:25Z

vault/identity_store_util.go

 	if err != nil {
 		return err
 	}

-	return nil
+	c.identityStore.logger.Warn("enabling case sensitive identity names")


Can we move this to L52? It would make the error from resetDB above more clear (i.e. error is related to switching over to handling case sensitive names).

calvn · 2018-10-04T19:39:25Z

vault/identity_store_util.go

-	return nil
+	c.identityStore.logger.Warn("enabling case sensitive identity names")
+
+	// Load everything from the beginning.


"Attempt to load artifacts once more after memdb is reset to accept case sensitive names" might be more clear since it was called already once before.

vault/identity_store_util.go

jefferai · 2018-10-10T14:06:45Z

vault/identity_store_aliases.go

@@ -181,10 +181,6 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc {
 			if entity == nil {
 				return nil, fmt.Errorf("existing alias is not associated with an entity")
 			}
-			if canonicalID == "" || entity.ID == canonicalID {


Why is this taken out?

This is to ensure that we always update the alias. This is useful when an existing alias name is updated with its casing altered.

jefferai · 2018-10-10T14:08:36Z

vault/identity_store_aliases.go

@@ -255,6 +251,12 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc {
 			return nil, err
 		}

+		for index, item := range entity.Aliases {


Same, can you explain what this change is for?

All the updates are held by the alias object. This is where we reflect those changes in the entity.

jefferai · 2018-10-10T14:11:31Z

vault/identity_store_util.go

+	case err == nil:
+		// If it succeeds, all is well
+		return nil
+	case err != nil && !errwrap.ContainsType(err, errDuplicateIdentityName):


I don't think ContainsType does what you think it does here, which makes me wonder if this works properly. ContainsType is supposed to be used to validate a typed error, which means this will likely match against any error generated via errors.New. You probably want .Contains()

Wow. This is a nice observation. Fixed. But the code was working properly because the code expecting a true and ContainsType (as you said) was returning true. There wasn't a different error there to catch. I tested it by overwriting the err returned from loadFunc and verified that its getting caught properly now.

jefferai · 2018-10-10T14:15:09Z

vault/identity_store_util.go

+			if err != nil {
+				return err
+			}
+			if groupByName != nil && !i.disableLowerCasedNames {


What happens if a group is found and lower caes names are disabled? That's also an error condition but won't be covered here.

When a group is found and when lower cased names are disabled, it means that we are falling back to the original behavior, and we are okay with the existence of another group. It is not an error condition. It would only mean that another group with same but differently cased name exists.

It would be good to warn in that case just as with entity names -- that way they can try to address the issue.

There was a warning with only one item that lead to the conflict. But your suggestion helps identify all the conflicting items after switching over to the case sensitive way. Nice observation. Fixed.

briankassouf

Just one very small comment, otherwise looks great!

briankassouf · 2018-10-19T16:39:51Z

vault/identity_store_util.go

+				return err
+			}
+			if groupByName != nil {
+				i.logger.Warn(errDuplicateIdentityName.Error(), "group_name", group.Name, "conflicting_group_name", groupByName.Name)


Could we give an actionable here and in the same log line bellow? Something like, please merge these groups or rename one

vishalnayak force-pushed the case-insensitive-identity-names branch from de0b089 to 523c727 Compare September 26, 2018 22:58

vishalnayak added this to the 0.11.2 milestone Sep 27, 2018

vishalnayak changed the title ~~[WIP] Case insensitive identity names~~ Case insensitive identity names Sep 27, 2018

tyrannosaurus-becks previously approved these changes Sep 27, 2018

View reviewed changes

vishalnayak dismissed tyrannosaurus-becks’s stale review via b11b66e September 28, 2018 02:11

kalafut reviewed Sep 28, 2018

View reviewed changes

vault/identity_store_util.go Outdated Show resolved Hide resolved

vishalnayak requested review from jefferai and briankassouf September 28, 2018 15:57

kalafut previously approved these changes Sep 28, 2018

View reviewed changes

calvn reviewed Sep 28, 2018

View reviewed changes

vishalnayak dismissed kalafut’s stale review via aed4b1b October 1, 2018 14:21

vishalnayak mentioned this pull request Oct 1, 2018

Canonical username as LDAP alias #5353

Closed

briankassouf reviewed Oct 1, 2018

View reviewed changes

chrishoffman modified the milestones: 0.11.2, next-release Oct 2, 2018

case insensitive identity names

750917d

vishalnayak force-pushed the case-insensitive-identity-names branch from 2eda462 to 750917d Compare October 2, 2018 21:18

calvn reviewed Oct 4, 2018

View reviewed changes

vishalnayak added 2 commits October 4, 2018 16:34

Merge branch 'master-oss' into case-insensitive-identity-names

c07c2b5

TestIdentityStore_GroupHierarchyCases

2412e4f

chrishoffman modified the milestones: next-release, 0.12 Oct 9, 2018

jefferai reviewed Oct 10, 2018

View reviewed changes

vishalnayak added 4 commits October 11, 2018 11:57

Merge branch 'master-oss' into case-insensitive-identity-names

242a2a3

address review feedback

328119f

Use errwrap.Contains instead of errwrap.ContainsType

f7f34c0

Warn about duplicate names all the time to help fix them

2e7f6ce

vishalnayak requested a review from briankassouf October 15, 2018 22:52

briankassouf previously approved these changes Oct 19, 2018

View reviewed changes

Address review feedback

50d3598

vishalnayak dismissed briankassouf’s stale review via 50d3598 October 19, 2018 17:00

vishalnayak merged commit fabe1d9 into master Oct 19, 2018

vishalnayak deleted the case-insensitive-identity-names branch October 19, 2018 19:47

jefferai modified the milestones: 0.12, 0.11.4 Oct 22, 2018

		@@ -214,6 +214,8 @@ func (i *IdentityStore) handleEntityUpdateCommon() framework.OperationFunc {

		if entityName != "" {
		entity.Name = entityName

Case insensitive identity names #5404

Case insensitive identity names #5404

Conversation

vishalnayak commented Sep 26, 2018 • edited Loading

vishalnayak commented Sep 27, 2018

tyrannosaurus-becks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kalafut commented Sep 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briankassouf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalnayak commented Sep 26, 2018 •

edited

Loading