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

Jump to bottom

[VAULT-3252] Disallow alias creation if entity/accessor combination exists #12747

Merged
merged 11 commits into from
Oct 14, 2021
Merged

[VAULT-3252] Disallow alias creation if entity/accessor combination exists #12747

merged 11 commits into from
Oct 14, 2021

Conversation

pmmukh
Copy link
Contributor

@pmmukh pmmukh commented Oct 5, 2021 •
edited by mickael-hc
Loading

  • This PR disallows entity alias creation if an alias for the specified entity and mount already exists, along with a test that checks the same
  • It disallows entity alias updates, if the alias is being changed to a mount for which its entity already has an alias, or if the alias is being associated to an entity which already has an alias for its mount
  • It also logs a warning when entities are loaded on Vault startup, to tackle the case of such existing duplicates, in order to let the operator know this setup should be addressed ( should the log be a bit clearer about that ? )
  • Existing tests that depended on the current behavior of creating multiple aliases on the same entity and for the same accessor have been updated. The behavior was removed entirely from TestIdentityStore_MergeEntitiesByID, as that did not actually require this behavior and works fine without it. TestCoreMetrics_EntityGauges and TestIdentityStore_CreateOrFetchEntity were updated to create the secondary alias on an entity against a different mount.

Screenshot of create api error
Screen Shot 2021-10-05 at 11 10 46 AM

Screenshot of startup log using a file backend

Screen Shot 2021-10-13 at 12 36 36 PM

@pmmukh pmmukh requested review from vishalnayak, a team and mickael-hc October 5, 2021 21:07
@vercel vercel bot temporarily deployed to Preview – vault October 5, 2021 21:12 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 5, 2021 21:12 Inactive
@swayne275
Copy link
Contributor

we may want to use something other than 400 for the duplicate request. perhaps 409? 400 is The server could not understand the request due to invalid syntax. which doesn't really seem to fit.

@chelshaw thoughts?

swayne275
swayne275 reviewed Oct 5, 2021
View reviewed changes
ccapurso
ccapurso suggested changes Oct 6, 2021
View reviewed changes
Copy link
Contributor

@ccapurso ccapurso 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. I left a couple small questions/suggestions.

@vercel vercel bot temporarily deployed to Preview – vault October 6, 2021 20:29 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 6, 2021 20:29 Inactive
@pmmukh pmmukh requested review from ccapurso and swayne275 October 6, 2021 20:30
@vercel vercel bot temporarily deployed to Preview – vault October 8, 2021 13:33 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 8, 2021 13:33 Inactive
@pmmukh pmmukh requested a review from ncabatoff October 8, 2021 13:35
@pmmukh
Copy link
Contributor Author

pmmukh commented Oct 8, 2021 •
edited
Loading

Looks like I've broken some existing tests with these changes, taking a look at those now

@vercel vercel bot temporarily deployed to Preview – vault-storybook October 8, 2021 15:26 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 8, 2021 15:26 Inactive
ncabatoff
ncabatoff reviewed Oct 13, 2021
View reviewed changes
changelog/12747.txt Outdated
@@ -0,0 +1,3 @@
```release-note:bug
core/identity: Disallow entity alias creation when an alias for the specified entity and mount exists
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since it's no longer just about creation, maybe reword?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

2d46fa2

@vercel vercel bot temporarily deployed to Preview – vault October 13, 2021 21:07 Inactive
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 13, 2021 21:07 Inactive
@pmmukh
Copy link
Contributor Author

pmmukh commented Oct 13, 2021

Just dropping a note that the updated TestCoreMetrics_EntityGauges is currently flaky, working on fixing that up now.

Fixed by 3c47f12

@pmmukh pmmukh requested a review from taoism4504 as a code owner October 13, 2021 21:35
@vercel vercel bot temporarily deployed to Preview – vault-storybook October 13, 2021 21:35 Inactive
ncabatoff
ncabatoff reviewed Oct 14, 2021
View reviewed changes
vault/identity_store_entities.go Outdated
@@ -799,6 +808,10 @@ func (i *IdentityStore) mergeEntity(ctx context.Context, txn *memdb.Txn, toEntit
return nil, fmt.Errorf("failed to update alias during merge: %w", err)
}

if _, ok := toEntityAccessors[alias.MountAccessor]; ok {
i.logger.Warn("skipping from_entity alias which has an accessor on which to_entity has an alias", "from_entity", fromEntityID, "skipped_alias", alias.ID)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this log message's context clear? Should we maybe add something to indicate that this is about an entity merge?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point! updated it here b465411

@vercel vercel bot temporarily deployed to Preview – vault-storybook October 14, 2021 16:17 Inactive
@vercel vercel bot temporarily deployed to Preview – vault October 14, 2021 16:17 Inactive
@pmmukh pmmukh merged commit 28bd5c3 into main Oct 14, 2021
@pmmukh pmmukh deleted the vault-3252-restrict-dupe-aliases branch October 14, 2021 16:52
pmmukh added a commit that referenced this pull request Oct 14, 2021
@pmmukh
[VAULT-3252] Disallow alias creation if entity/accessor combination e…
0a1e99d 
…xists (#12747)

* Disallow alias creation if entity/accessor combination exists

* Add changelog

* Address review comments

* Add handling to aliasUpdate, some field renaming

* Update tests to work under new entity-alias constraint

* Add check to entity merge, other review fixes

* Log duplicated accessors only once

* Fix flaky test

* Add note about new constraint to docs

* Update entity merge warn log
@pmmukh pmmukh mentioned this pull request Oct 14, 2021
Merged
@pmmukh pmmukh added this to the 1.9 milestone Oct 14, 2021
pmmukh added a commit that referenced this pull request Oct 14, 2021
@pmmukh
[VAULT-3252] Disallow alias creation if entity/accessor combination e…
4a1b45b 
…xists (#12747)

* Disallow alias creation if entity/accessor combination exists

* Add changelog

* Address review comments

* Add handling to aliasUpdate, some field renaming

* Update tests to work under new entity-alias constraint

* Add check to entity merge, other review fixes

* Log duplicated accessors only once

* Fix flaky test

* Add note about new constraint to docs

* Update entity merge warn log
@pmmukh pmmukh mentioned this pull request Oct 14, 2021
Merged
pmmukh added a commit that referenced this pull request Oct 14, 2021
@pmmukh
[VAULT-3252] Disallow alias creation if entity/accessor combination e…
3e83f9b 
…xists (#12747)

* Disallow alias creation if entity/accessor combination exists

* Add changelog

* Address review comments

* Add handling to aliasUpdate, some field renaming

* Update tests to work under new entity-alias constraint

* Add check to entity merge, other review fixes

* Log duplicated accessors only once

* Fix flaky test

* Add note about new constraint to docs

* Update entity merge warn log
@pmmukh pmmukh mentioned this pull request Oct 14, 2021
Merged
pmmukh added a commit that referenced this pull request Oct 14, 2021
@pmmukh
[VAULT-3252] Disallow alias creation if entity/accessor combination e…
5fbc168 
…xists (#12747) (#12836)

* Disallow alias creation if entity/accessor combination exists

* Add changelog

* Address review comments

* Add handling to aliasUpdate, some field renaming

* Update tests to work under new entity-alias constraint

* Add check to entity merge, other review fixes

* Log duplicated accessors only once

* Fix flaky test

* Add note about new constraint to docs

* Update entity merge warn log
pmmukh added a commit that referenced this pull request Oct 14, 2021
@pmmukh
[VAULT-3252] Disallow alias creation if entity/accessor combination e…
6a575c6 
…xists (#12747) (#12837)

* Disallow alias creation if entity/accessor combination exists

* Add changelog

* Address review comments

* Add handling to aliasUpdate, some field renaming

* Update tests to work under new entity-alias constraint

* Add check to entity merge, other review fixes

* Log duplicated accessors only once

* Fix flaky test

* Add note about new constraint to docs

* Update entity merge warn log
pmmukh added a commit that referenced this pull request Oct 14, 2021
@pmmukh
Backport pr 12747 1.6 (#12838)
b01189a 
* [VAULT-3252] Disallow alias creation if entity/accessor combination exists (#12747)

* Disallow alias creation if entity/accessor combination exists

* Add changelog

* Address review comments

* Add handling to aliasUpdate, some field renaming

* Update tests to work under new entity-alias constraint

* Add check to entity merge, other review fixes

* Log duplicated accessors only once

* Fix flaky test

* Add note about new constraint to docs

* Update entity merge warn log

* Add doc update
@christophermaier
Copy link

Sorry to revive an old-ish thread, but is there a link to information for why this change was made? I previously had multiple aliases within a given accessor to the same entity, and now need to re-work how I'm representing things.

I would just like to understand what was wrong about having multiple aliases. Thanks in advance! 🙇

@pmmukh
Copy link
Contributor Author

pmmukh commented Jan 20, 2022

hey @christophermaier ! Sure thing, so the issue was that the behavior could be exploited via a vulnerability, you can find the CVE noted here https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2021-43998, and some details around why we went with this solution to fix the cve in the 1.9 upgrade guide here https://www.vaultproject.io/docs/upgrading/upgrade-to-1.9.x#entity-alias-mapping.

@christophermaier
Copy link

@pmmukh That makes sense; thanks for the links and for the very quick response! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raskchanky raskchanky raskchanky approved these changes

@ccapurso ccapurso ccapurso approved these changes

@vishalnayak vishalnayak vishalnayak approved these changes

@ncabatoff ncabatoff ncabatoff approved these changes

@mickael-hc mickael-hc Awaiting requested review from mickael-hc

@swayne275 swayne275 Awaiting requested review from swayne275

@taoism4504 taoism4504 Awaiting requested review from taoism4504

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
1.9
Development

Successfully merging this pull request may close these issues.
7 participants
@pmmukh @swayne275 @christophermaier @raskchanky @ccapurso @vishalnayak @ncabatoff