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

VAULT-6818 - Restrict ability to merge entities with mount-accessor-conflicting aliases unless one is explicitly chosen to be kept #16539

Merged
merged 17 commits into from
Aug 10, 2022

Conversation

VioletHynes
Copy link
Contributor

@VioletHynes VioletHynes commented Aug 2, 2022

In the case that there are conflicting aliases, we will error and print out a list of alias that conflict. This error could look a little like this:

* conflicting alias mount accessors between toEntity and fromEntity, clashes:
mountAccessor: auth_github_2972056c, toEntity ID: 131bcbc3-ddbc-0e2f-cb4f-62b893ec2332, fromEntity ID: 723a0466-c4a8-0cf1-1b7d-ddb8249fedb6, conflicting toEntity alias ID: 128cad0e-53b6-907c-3de4-c9dfd0293296, conflicting fromEntity alias ID: 22aa3d3d-0fdd-e0a4-20c5-7e6d81dce299
mountAccessor: auth_userpass_6c75ad09, toEntity ID: 131bcbc3-ddbc-0e2f-cb4f-62b893ec2332, fromEntity ID: 879ecf5c-c628-6088-1d64-38c1c87142f5, conflicting toEntity alias ID: 7c371e42-b2bd-bf9c-a990-afde0d04b516, conflicting fromEntity alias ID: 83dea9e3-f979-c900-6613-e4bf15e5a220

To merge such a set of aliases, you must:

  • Provide only a single from_entity_id

  • Provide a list of the alias ids you want to keep as conflicting_alias_ids_to_keep (the others will be deleted)

Test cases cover the following:

  • Merging entities, deleting the entity, then logging in using an old alias (The originally reported bug)
  • Merging entities A and B where A and B have clashing aliases
  • Merging entities B and C into A where B and C have clashing aliases
  • Merging entities B and C into A where both B and C have clashing aliases with A
  • Merging entities B and C into A with the conflicting_alias_ids_to_keep value, and ensuring an error (only one clash merge allowed at once)

Docs PR here: #16593

@VioletHynes VioletHynes marked this pull request as ready for review August 2, 2022 20:16
Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

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

Looks good so far -- I really like all the testing and thorough comments, as well as the log lines!

vault/identity_store_entities.go Show resolved Hide resolved
vault/identity_store_entities.go Show resolved Hide resolved
vault/identity_store_entities.go Outdated Show resolved Hide resolved
vault/identity_store_entities.go Outdated Show resolved Hide resolved
changelog/16539.txt Outdated Show resolved Hide resolved
Copy link
Collaborator

@ncabatoff ncabatoff left a comment

Choose a reason for hiding this comment

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

Are any docs updates needed?

@VioletHynes
Copy link
Contributor Author

Are any docs updates needed?

Definitely will be docs updates needed. They'll come as a PR after this one!

vault/identity_store_entities.go Outdated Show resolved Hide resolved
vault/identity_store_entities.go Show resolved Hide resolved
vault/identity_store_entities.go Outdated Show resolved Hide resolved
vault/identity_store_entities.go Outdated Show resolved Hide resolved
vault/identity_store_entities.go Outdated Show resolved Hide resolved
vault/identity_store_entities.go Show resolved Hide resolved
vault/identity_store_entities.go Outdated Show resolved Hide resolved
vault/identity_store_entities.go Outdated Show resolved Hide resolved
@VioletHynes
Copy link
Contributor Author

#16539 (comment)

(For some reason I can't respond to this comment, so adding my own and linking)

The reason for the continue is that the other path shares the merge code with the normal, non-clash case. It would be possible to duplicate the code and move the continue to the other code path (or add some kind of check), but I don't think there's much getting around a little awkwardness here

@HridoyRoy HridoyRoy dismissed their stale review August 8, 2022 18:01

Chatted about this in person!

Copy link
Contributor

@HridoyRoy HridoyRoy left a comment

Choose a reason for hiding this comment

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

Changes look good to me! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants