Entity Alias transfer corrupts receiving entity?

[stephanh]

When an entity-alias is updated to alias a different entity. The receiving entity now has two copies of the alias. Here is an example:

[
  {
    "canonical_id": "6f925fa9-5207-e4a8-c880-d5c5ffe2df38",
    "creation_time": "2018-11-08T03:42:19.538526Z",
    "id": "bd7f178e-990c-4750-18b7-3f4db4bb2692",
    "last_update_time": "2018-11-08T03:44:01.420729Z",
    "merged_from_canonical_ids": null,
    "metadata": null,
    "mount_accessor": "auth_token_aa28d9b6",
    "mount_path": "auth/token/",
    "mount_type": "token",
    "name": "alias1"
  },
  {
    "canonical_id": "6f925fa9-5207-e4a8-c880-d5c5ffe2df38",
    "creation_time": "2018-11-08T03:42:19.538526Z",
    "id": "bd7f178e-990c-4750-18b7-3f4db4bb2692",
    "last_update_time": "2018-11-08T03:42:19.538526Z",
    "merged_from_canonical_ids": [
      "83237a90-6fa4-b487-f567-d724ff489e06"
    ],
    "metadata": null,
    "mount_accessor": "auth_token_aa28d9b6",
    "mount_path": "auth/token/",
    "mount_type": "token",
    "name": "alias1"
  }
]

I believe this is also the root cause for #5348. Prior to 0.11.4 it was impossible to delete the transferred alias.

To Reproduce
Steps to reproduce the behavior:

# Create an entity-alias without linking it a specific entity
(vault-dev) bash-4.4$ vault write identity/entity-alias name=alias1 mount_accessor=auth_token_aa28d9b6
Key             Value
---             -----
canonical_id    83237a90-6fa4-b487-f567-d724ff489e06
id              bd7f178e-990c-4750-18b7-3f4db4bb2692

# Create an entity
(vault-dev) bash-4.4$ vault write identity/entity name=entity1
Key        Value
---        -----
aliases    <nil>
id         6f925fa9-5207-e4a8-c880-d5c5ffe2df38

# Update alias1 to link it to entity1
(vault-dev) bash-4.4$ vault write identity/entity-alias/id/bd7f178e-990c-4750-18b7-3f4db4bb2692 canonical_id=6f925fa9-5207-e4a8-c880-d5c5ffe2df38 name=alias1 mount_accessor=auth_token_aa28d9b6
WARNING! The following warnings were returned from Vault:

  * alias is being transferred from entity
  "83237a90-6fa4-b487-f567-d724ff489e06" to
  "6f925fa9-5207-e4a8-c880-d5c5ffe2df38"

Key             Value
---             -----
canonical_id    6f925fa9-5207-e4a8-c880-d5c5ffe2df38
id              bd7f178e-990c-4750-18b7-3f4db4bb2692

# Read entity1 and see that alias1 is now mentioned twice.
(vault-dev) bash-4.4$ vault read --format=json identity/entity/id/6f925fa9-5207-e4a8-c880-d5c5ffe2df38 | jq .data.aliases
[
  {
    "canonical_id": "6f925fa9-5207-e4a8-c880-d5c5ffe2df38",
    "creation_time": "2018-11-08T03:42:19.538526Z",
    "id": "bd7f178e-990c-4750-18b7-3f4db4bb2692",
    "last_update_time": "2018-11-08T03:44:01.420729Z",
    "merged_from_canonical_ids": null,
    "metadata": null,
    "mount_accessor": "auth_token_aa28d9b6",
    "mount_path": "auth/token/",
    "mount_type": "token",
    "name": "alias1"
  },
  {
    "canonical_id": "6f925fa9-5207-e4a8-c880-d5c5ffe2df38",
    "creation_time": "2018-11-08T03:42:19.538526Z",
    "id": "bd7f178e-990c-4750-18b7-3f4db4bb2692",
    "last_update_time": "2018-11-08T03:42:19.538526Z",
    "merged_from_canonical_ids": [
      "83237a90-6fa4-b487-f567-d724ff489e06"
    ],
    "metadata": null,
    "mount_accessor": "auth_token_aa28d9b6",
    "mount_path": "auth/token/",
    "mount_type": "token",
    "name": "alias1"
  }
]

Expected behavior
The receiving entity should only have one reference to the alias.

Environment:

• Vault Server Version (retrieve with vault status): 0.11.4
• Vault CLI Version (retrieve with vault version): 0.11.4
• Server Operating System/Architecture: Mac and Linux

Potential workaround

This problem can be avoided by instead first deleting the entity-alias and then recreating with the canonical_id field set instead of updating it. Using this approach means that the first created entity won't be deleted though, while on transfer the first created entity is deleted. I'm not sure what the implications of that are.

[jefferai]

Consolidated repro case:

aliasid=$(vault auth list -format=json | jq -r '.["token/"].accessor' | vault write -field=id identity/entity-alias name=alias1 mount_accessor=-)
e=$(vault read -field=canonical_id identity/entity-alias/id/${aliasid})
vault read identity/entity/id/${e}
entityid=$(vault write -field=id identity/entity name=entity1)
vault read identity/entity/id/${entityid}
vault read identity/entity-alias/id/${aliasid}
vault write identity/entity-alias/id/${aliasid} canonical_id=${entityid}
vault read --format=json identity/entity/id/${entityid} | jq .data.aliases

[jefferai]

@vishalnayak this workflow is a bit confusing to me. If the alias is already tied to another entity, why do the entities get merged from attempting to update the alias? It seems like that shouldn't happen unless you specifically call identity/entity/merge.

I'm basically wondering if the issue is really when we're merging when we shouldn't. So in a normal merge triggered by identity/entity/merge, the aliases would be different to begin with so appending them is correct. But in this case instead of assigning an entity to a different entity, it's trying to merge the two together.

[jefferai]

There seem to be a few issues at work here.

One is that handleAliasUpdateCommon doesn't take into account a scenario where you haven't found an alias due to given factors -- for instance if you don't provide a mount accessor. In that case you'll have an alias found by ID, but no aliasByFactors, which means entity will stay nil. In the next block canonicalID will be non-"" so you'll go into the else condition, but after looking for a canonicalEntity, entity here will still be nil. So we will append the alias to it, rather than hit the else block where we'd treat it as a migration, performing a move. This will effectively give us the same alias bound to two entities.

This diff should (I think) fix that problem:

diff --git a/vault/identity_store_aliases.go b/vault/identity_store_aliases.go
index e81851283..84c7f717f 100644
--- a/vault/identity_store_aliases.go
+++ b/vault/identity_store_aliases.go
@@ -181,6 +181,13 @@ func (i *IdentityStore) handleAliasUpdateCommon() framework.OperationFunc {
                        if entity == nil {
                                return nil, fmt.Errorf("existing alias is not associated with an entity")
                        }
+               } else if alias.ID != "" {
+                       // This is an update, not a create; if we have an associated entity
+                       // already, load it
+                       entity, err = i.MemDBEntityByAliasID(alias.ID, true)
+                       if err != nil {
+                               return nil, err
+                       }
                }
 
                resp := &logical.Response{}

Second problem: in upsertEntityInTxn it does a check to see if an alias being inserted matches an existing alias. However, note how upsertEntityInTxn takes in both entity and previousEntity but previousEntity has not yet been upserted -- that's done in this function, and as a result the indexes of the aliases that formerly resided in previousEntity will not have been cleared. So when it checks aliases, the aliases that were just added to the entity being passed in will also succeed a lookup in memdb!

Fixing this is a bit tricker. I think what would have to happen is, if we hit the case where we found an aliasByFactors, we'd have to also check if the alias's canonical ID is either a) not a match to previousEntity.ID (if previousEntity isn't nil), or b) if it is a match, check to see whether previousEntity.Aliases no longer contains an alias having the matching ID and if so ignore it.

Finally, back to merging behavior. Let's say that we in fact don't find the factors from an old index from previousEntity but somehow some other entity. I still am not sure that merging is actually the right thing to do in this case. It seems like we should not be automatically merging entities just because we found a duplicate alias, but I'm not sure what the original reasoning was behind this, or if was a bug introduced at some point.

I think fixing the first two issues will make the third less/not likely to ever occcur, but that doesn't mean we shouldn't change that behavior anyways unless there's a good reason to keep it.