From 5e34be2c683dc975588e6aace0e5456c196cc101 Mon Sep 17 00:00:00 2001 From: jeanluc Date: Sun, 8 Dec 2024 17:08:31 +0100 Subject: [PATCH] Create separate metadata entries for composite values --- changelog/+listcrash.fixed.md | 1 + changelog/+rolesmetadata.added.md | 1 + docs/topics/templating.md | 60 ++++++++++++++++--- src/saltext/vault/runners/vault.py | 48 ++++++++++++++- src/saltext/vault/utils/vault/helpers.py | 15 +++-- .../files/vault/policies/salt_minion.hcl | 11 ++++ tests/integration/runners/test_vault.py | 19 +++++- tests/unit/runners/vault/test_vault.py | 33 +++++++++- 8 files changed, 165 insertions(+), 23 deletions(-) create mode 100644 changelog/+listcrash.fixed.md create mode 100644 changelog/+rolesmetadata.added.md diff --git a/changelog/+listcrash.fixed.md b/changelog/+listcrash.fixed.md new file mode 100644 index 00000000..cb510e7b --- /dev/null +++ b/changelog/+listcrash.fixed.md @@ -0,0 +1 @@ +Fixed a crash when a templated field accesses an out-of-bounds list index diff --git a/changelog/+rolesmetadata.added.md b/changelog/+rolesmetadata.added.md new file mode 100644 index 00000000..2a7c19e4 --- /dev/null +++ b/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 diff --git a/docs/topics/templating.md b/docs/topics/templating.md index c6d000fb..c0585a3c 100644 --- a/docs/topics/templating.md +++ b/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 ` 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,7 +82,7 @@ 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 @@ -90,7 +90,49 @@ will be expanded to: ``` 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"] +} +``` diff --git a/src/saltext/vault/runners/vault.py b/src/saltext/vault/runners/vault.py index 5137e31f..2b24ec40 100644 --- a/src/saltext/vault/runners/vault.py +++ b/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()} diff --git a/src/saltext/vault/utils/vault/helpers.py b/src/saltext/vault/utils/vault/helpers.py index 8419197e..98d1cb4e 100644 --- a/src/saltext/vault/utils/vault/helpers.py +++ b/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]``. diff --git a/tests/integration/files/vault/policies/salt_minion.hcl b/tests/integration/files/vault/policies/salt_minion.hcl index 1c52b5aa..11ea0835 100644 --- a/tests/integration/files/vault/policies/salt_minion.hcl +++ b/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"] diff --git a/tests/integration/runners/test_vault.py b/tests/integration/runners/test_vault.py index 0912f5c5..9344c32b 100644 --- a/tests/integration/runners/test_vault.py +++ b/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") diff --git a/tests/unit/runners/vault/test_vault.py b/tests/unit/runners/vault/test_vault.py index eecd7dcc..3daba52f 100644 --- a/tests/unit/runners/vault/test_vault.py +++ b/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")