Create separate metadata entries for composite values

changelog/+listcrash.fixed.md

@@ -0,0 +1 @@
+Fixed a crash when a templated field accesses an out-of-bounds list index

changelog/+rolesmetadata.added.md

@@ -0,0 +1 @@
+When metadata that is written to Vault is templated using a list or dict, in addition to concatenating the values into a sorted comma-separated list, the master now additionally creates a separate suffixed key for each individual item

docs/topics/templating.md

@@ -24,20 +24,20 @@ Generally available template variables are:
       :::{important}
       Pillar values sourced from Vault should never be referenced here:
 
-      * In the general case, they will be undefined to prevent circular references.
+      * In the general case, they are undefined to prevent circular references.
         If you use the Vault integration during rendering of some your regular pillar
-        `sls` files, all values from these will be undefined, even the ones that do not
+        `sls` files, all values originating from these are undefined, even the ones that do not
         depend on Vault. It is thus highly encouraged to avoid calling the Vault modules
         during pillar rendering if you rely on pillar templating in this extension.
         You can use the dedicated Vault pillar module or reference secrets during
         **state** rendering instead, possibly in conjunction with map files
         to avoid hardcoding Vault sources in the states themselves.
-      * In some cases, a cached pillar will be used for performance
+      * In some cases, a cached pillar is used for performance
         reasons, which can contain Vault-sourced values. This only happens during
         **token** (not AppRole) policy rendering and can be disabled by setting
         {vconf}`policies:refresh_pillar <policies:refresh_pillar>` to `true`.
 
-      Pillar values from previously rendered pillars can be used to template
+      Pillar values originating from previously rendered pillars can be used to template
       [Vault Pillar](saltext.vault.pillar.vault) paths. Using pillar values to template
       Vault pillar paths requires them to be defined before the Vault ext_pillar is called.
       Especially consider the significancy of the
@@ -82,15 +82,57 @@ roles_map:
 ```
 
 A pattern of `salt_role_{pillar[roles]}` (or `salt_role_{pillar[roles_map]}`)
-will be expanded to:
+is expanded to:
 
 ```yaml
 - salt_role_mail
 - salt_role_web
 ```
 
 How this is used depends on the scope.
-* Policies and pillar paths will use the values separately, as intended.
-* Entity and authentication metadata is written to Vault, which does not
-  support complex values. The expanded values are thus concatenated into
-  a sorted comma-separated list.
+
+#### Policies/pillar paths
+Policies and pillar paths will use the values separately, as intended.
+
+#### Entity/authentication metadata
+Entity and authentication metadata is written to Vault, which [does not
+support complex values](https://github.com/hashicorp/vault/issues/8570#issuecomment-778585231).
+
+There is an inelegant workaround in place for this behavior though. Assuming the following master configuration:
+```yaml
+vault:
+  metadata:
+    entity:
+      roles: '{pillar[roles]}'
+```
+
+* The configured metadata key (`roles`) will contain all values concatenated into a sorted comma-separated list.
+* Additionally, for each value in the list, a separate metadata key is added containing only a single value. Its name
+  is the concatenation of the configured key, a double underscore and the list item's index.
+
+Thus, a minion with `pillar[roles]` == `[web, db]` receives the following metadata:
+
+```yaml
+roles: db,web
+roles__0: db
+roles__1: web
+```
+
+This allows to rely on templated Vault policies even when composite values are in play, which would
+usually need to be solved by templating {vconf}`policies:assign` and creating regular policies instead.
+
+In the templated policy, repeat each definition for as many times
+as the number of items you want to support, only changing the metadata key index:
+
+```vaultpolicy
+# This policy respects 3 different roles per minion at most
+path "salt/data/roles/{{identity.entity.metadata.roles__0}}" {
+    capabilities = ["read"]
+}
+path "salt/data/roles/{{identity.entity.metadata.roles__1}}" {
+  capabilities = ["read"]
+}
+path "salt/data/roles/{{identity.entity.metadata.roles__2}}" {
+  capabilities = ["read"]
+}
+```

src/saltext/vault/runners/vault.py

@@ -1035,19 +1035,61 @@ def _get_metadata(minion_id, metadata_patterns, refresh_pillar=None):
     metadata = {}
     for key, pattern in metadata_patterns.items():
         metadata[key] = []
+        composite_prefix = f"{key}__"
+
+        # First, expand templates that make use of composite values (recursively)
+        # until we have a list of patterns with simple values.
         try:
-            for expanded_pattern in helpers.expand_pattern_lists(pattern, **mappings):
-                metadata[key].append(expanded_pattern.format(**mappings))
-        except KeyError:
+            expanded_patterns = helpers.expand_pattern_lists(pattern, **mappings)
+        except (IndexError, KeyError):
+            # If any template in the chain fails to render, skip all of them.
+            # This is inherited from the old code and might be changed in a
+            # new major release @FIXME
             log.warning(
                 "Could not resolve metadata pattern %s for minion %s",
                 pattern,
                 minion_id,
             )
+            expanded_patterns = []
+
+        # Then substitute these simple template variables with their values.
+        for expanded_pattern in expanded_patterns:
+            try:
+                metadata[key].append(expanded_pattern.format(**mappings))
+            except (IndexError, KeyError):
+                log.warning(
+                    "Could not resolve metadata pattern %s for minion %s",
+                    pattern,
+                    minion_id,
+                )
+
         # Since composite values are disallowed for metadata,
         # at least ensure the order of the comma-separated string
         # is predictable
         metadata[key].sort()
+        # ... and assign the individual values to keys suffixed with their respective index.
+        # A corresponding policy would need to account for this behavior by repeating
+        # each assignment:
+        # path "salt/data/roles/{{identity.entity.metadata.role__0}}" {capabilities = ["read"]}
+        # path "salt/data/roles/{{identity.entity.metadata.role__1}}" {capabilities = ["read"]}
+        # ...
+        if expanded_patterns == [pattern]:
+            # Don't expand when the template did not involve a composite value.
+            pass
+        elif conflicting_keys := [
+            mkey for mkey in metadata_patterns if mkey.startswith(composite_prefix)
+        ]:
+            # Don't risk overwriting explicitly set keys. It's unlikely, but check regardless.
+            log.warning(
+                "Skipping list expansion of entity metadata '%s' for minion '%s': "
+                "Detected potentially conflicting key(s): %s'",
+                key,
+                minion_id,
+                conflicting_keys,
+            )
+        else:
+            for idx, value in enumerate(metadata[key]):
+                metadata[f"{composite_prefix}{idx}"] = [value]
 
     log.debug("%s metadata: %s", minion_id, metadata)
     return {k: ",".join(v) for k, v in metadata.items()}

src/saltext/vault/utils/vault/helpers.py

@@ -85,20 +85,19 @@ def expand_pattern_lists(pattern, **mappings):
     length N in the mappings present in the pattern, N copies of the pattern are
     returned, each with an element of the list substituted.
 
-    pattern:
-        A pattern to expand, for example ``by-role/{grains[roles]}``
+    pattern
+        A pattern to expand, for example ``by-role/{pillar[roles]}``
 
-    mappings:
+    mappings
         A dictionary of variables that can be expanded into the pattern.
 
-    Example: Given the pattern `` by-role/{grains[roles]}`` and the below grains
+    Example: Given the pattern `` by-role/{pillar[roles]}`` and the below pillar
 
     .. code-block:: yaml
 
-        grains:
-            roles:
-                - web
-                - database
+        roles:
+          - web
+          - database
 
     This function will expand into two patterns,
     ``[by-role/web, by-role/database]``.

tests/integration/files/vault/policies/salt_minion.hcl

@@ -18,6 +18,17 @@ path "salt/data/roles/{{identity.entity.metadata.role}}" {
     capabilities = ["read"]
 }
 
+# ACL policy templating tests with list-valued metadata
+path "salt/data/roles/{{identity.entity.metadata.roles__0}}" {
+    capabilities = ["read"]
+}
+path "salt/data/roles/{{identity.entity.metadata.roles__1}}" {
+    capabilities = ["read"]
+}
+path "salt/data/roles/{{identity.entity.metadata.roles__2}}" {
+    capabilities = ["read"]
+}
+
 # Test list policies
 path "sys/policy" {
     capabilities = ["read"]

tests/integration/runners/test_vault.py

@@ -504,7 +504,6 @@ def pillar_roles_tree(
     roles:
       - dev
       - web
-    # this is for entity metadata since lists are cumbersome at best
     role: foo
     """
     top_file = vault_salt_master.pillar_tree.base.temp_file("top.sls", top_pillar_contents)
@@ -517,12 +516,19 @@ def pillar_roles_tree(
 @pytest.fixture(scope="class")
 def vault_pillar_values_approle(vault_salt_minion):
     vault_write_secret(f"salt/minions/{vault_salt_minion.id}", minion_id_acl_template="worked")
+    # template is a single value
     vault_write_secret("salt/roles/foo", pillar_role_acl_template="worked")
+    # template expands to multiple values, templated policy relies on workaround
+    # that expands the keys (roles__0, roles__1, ...)
+    vault_write_secret("salt/roles/dev", pillar_roles_0_acl_template="worked")
+    vault_write_secret("salt/roles/web", pillar_roles_1_acl_template="worked")
     try:
         yield
     finally:
         vault_delete_secret(f"salt/minions/{vault_salt_minion.id}")
         vault_delete_secret("salt/roles/foo")
+        vault_delete_secret("salt/roles/dev")
+        vault_delete_secret("salt/roles/web")
 
 
 @pytest.fixture
@@ -606,7 +612,13 @@ def entities_synced(
     assert f"salt_minion_{vault_salt_minion.id}" in ret.data
     ret = vault_salt_run_cli.run("vault.show_entity", vault_salt_minion.id)
     assert ret.returncode == 0
-    assert ret.data == {"minion-id": vault_salt_minion.id, "role": "foo"}
+    assert ret.data == {
+        "minion-id": vault_salt_minion.id,
+        "role": "foo",
+        "roles": "dev,web",
+        "roles__0": "dev",
+        "roles__1": "web",
+    }
     yield
 
 
@@ -656,6 +668,7 @@ def vault_master_config(self, pillar_state_tree, vault_port):
                     "entity": {
                         "minion-id": "{minion}",
                         "role": "{pillar[role]}",
+                        "roles": "{pillar[roles]}",
                     },
                 },
                 "policies": {
@@ -733,6 +746,8 @@ def test_minion_pillar_is_populated_as_expected(self, vault_salt_call_cli):
         assert ret.data
         assert ret.data.get("minion_id_acl_template") == "worked"
         assert ret.data.get("pillar_role_acl_template") == "worked"
+        assert ret.data.get("pillar_roles_0_acl_template") == "worked"
+        assert ret.data.get("pillar_roles_1_acl_template") == "worked"
 
     @pytest.mark.usefixtures("approles_synced")
     @pytest.mark.usefixtures("conn_cache_absent")

tests/unit/runners/vault/test_vault.py

@@ -248,6 +248,7 @@ def pillar():
     return {
         "mixedcase": "UP-low-UP",
         "role": "test",
+        "roles": ["foo", "bar"],
     }
 
 
@@ -1019,6 +1020,14 @@ def test_get_policies_for_nonexisting_minions(expected):
             {"pillar-rendering:{pillar[role]}": "pillar-rendering:{pillar[role]}"},
             {"pillar-rendering:{pillar[role]}": "pillar-rendering:test"},
         ),
+        (
+            {"list-val": "list-val:{pillar[roles][0]}"},
+            {"list-val": "list-val:foo"},
+        ),
+        (
+            {"list-val-out-of-bounds": "list-val-out-of-bounds:{pillar[roles][10]}"},
+            {"list-val-out-of-bounds": ""},
+        ),
     ],
 )
 def test_get_metadata(metadata_patterns, expected, pillar):
@@ -1053,7 +1062,29 @@ def test_get_metadata_list():
                 {"salt_role": "salt_role_{pillar[roles]}"},
                 refresh_pillar=False,
             )
-            assert res == {"salt_role": "salt_role_bar,salt_role_foo"}
+            assert res == {
+                "salt_role": "salt_role_bar,salt_role_foo",
+                "salt_role__0": "salt_role_bar",
+                "salt_role__1": "salt_role_foo",
+            }
+
+
+def test_get_metadata_list_conflict():
+    with patch("salt.utils.minions.get_minion_data", autospec=True) as get_minion_data:
+        get_minion_data.return_value = (None, None, None)
+        with patch(
+            "saltext.vault.utils.vault.helpers.expand_pattern_lists", autospec=True
+        ) as expand:
+            expand.side_effect = lambda x, *args, **kwargs: ["foo", "bar"] if "pillar" in x else [x]
+            res = vault._get_metadata(
+                "test-minion",
+                {"salt_role": "salt_role_{pillar[roles]}", "salt_role__1": "wat"},
+                refresh_pillar=False,
+            )
+            assert res == {
+                "salt_role": "bar,foo",
+                "salt_role__1": "wat",
+            }
 
 
 @pytest.mark.usefixtures("config")