From ab3fbc3b2993db13b45088f0c46e318daac2e2c8 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/106.added.md | 1 + docs/ref/configuration.md | 76 ++++++++++------- docs/topics/templating.md | 84 ++++++++++++++++--- src/saltext/vault/runners/vault.py | 47 ++++++++++- src/saltext/vault/utils/vault/helpers.py | 15 ++-- .../files/vault/policies/salt_minion.hcl | 11 +++ tests/integration/runners/test_vault.py | 20 ++++- tests/unit/runners/vault/test_vault.py | 33 +++++++- 9 files changed, 234 insertions(+), 54 deletions(-) create mode 100644 changelog/+listcrash.fixed.md create mode 100644 changelog/106.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/106.added.md b/changelog/106.added.md new file mode 100644 index 00000000..2a7c19e4 --- /dev/null +++ b/changelog/106.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/ref/configuration.md b/docs/ref/configuration.md index 0128a4cb..4bc59fe3 100644 --- a/docs/ref/configuration.md +++ b/docs/ref/configuration.md @@ -75,7 +75,7 @@ Token renewal settings. :::{note} This setting can be specified inside a minion's configuration as well -and will override the master's default for the minion. +and then overrides the master's default for the minion. Token lifecycle settings have significance for any authentication method, not just `token`. @@ -86,9 +86,9 @@ not just `token`. minimum_ttl : Specifies the time (in seconds or as a time string like `24h`) an in-use token should be valid for. If the current validity period is less than this - and the token is renewable, a renewal will be attempted. If it is not renewable + and the token is renewable, a renewal is attempted. If it is not renewable or a renewal does not extend the ttl beyond the specified minimum, a new token - will be generated. + is generated. :::{hint} Since leases like database credentials are tied to a token, setting this to @@ -100,7 +100,7 @@ minimum_ttl ::: renew_increment : Specifies the amount of time the token's validity should be requested to be - renewed for when renewing a token. When unset, will extend the token's validity + renewed for when renewing a token. When unset, extends the token's validity by its default ttl. Set this to `false` to disable token renewals. :::{note} @@ -116,11 +116,11 @@ configuration cache on minions that receive issued credentials. :::{vconf} cache:backend ::: #### backend -The cache backend in use. Defaults to `session`, which will store the +The cache backend in use. Defaults to `session`, which stores the Vault configuration in memory only for that specific Salt run. -`disk`/`file`/`localfs` will force using the localfs driver, regardless +`disk`/`file`/`localfs` force using the localfs driver, regardless of configured minion data cache. -Setting this to anything else will use the default configured cache for +Setting this to anything else uses the default configured cache for minion data ({conf_master}`cache `), by default the local filesystem as well. @@ -138,7 +138,7 @@ When encountering an `Unauthorized` response with an otherwise valid token, flush the cache and request new credentials. Defaults to true. :::{tip} -If your policies are relatively stable, disabling this will prevent +If your policies are relatively stable, disabling this prevents a lot of unnecessary overhead, with the tradeoff that once they change, you might have to clear the cache manually or wait for the token to expire. ::: @@ -148,13 +148,13 @@ you might have to clear the cache manually or wait for the token to expire. #### config The time in seconds to cache queried configuration from the master. Defaults to `3600` (one hour). Set this to `null` to disable -cache expiration. Changed {vconf}`server` configuration on the master will -still be recognized, but changes in {vconf}`auth` and {vconf}`cache` will need +cache expiration. Changed {vconf}`server` configuration on the master +is still recognized, but changes in {vconf}`auth` and {vconf}`cache` need a manual update using [vault.update_config](saltext.vault.modules.vault.update_config) or cache clearance using [vault.clear_cache](saltext.vault.modules.vault.clear_cache). :::{note} -Expiring the configuration will also clear cached authentication +Expiring the configuration also clears cached authentication credentials and leases. ::: @@ -171,9 +171,9 @@ Defaults to false. ::: #### kv_metadata The time in seconds to cache KV metadata used to determine if a path -is using version 1/2 for. Defaults to `connection`, which will clear +is using version 1/2 for. Defaults to `connection`, which clears the metadata cache once a new configuration is requested from the -master. Setting this to `null` will keep the information +master. Setting this to `null` keeps the information indefinitely until the cache is cleared manually using [vault.clear_cache](saltext.vault.modules.vault.clear_cache) with `connection=false`. @@ -308,7 +308,7 @@ via [enable_rate_limit_response_headers](https://developer.hashicorp.com/vault/a :::{versionadded} 1.1.0 ::: When {vconf}`respect_retry_after ` is True, limit -the maximum amount of seconds the client will sleep before retrying. Set this to `null` (YAML/JSON)/`None` (Python) +the maximum amount of seconds the client sleeps before retrying. Set this to `null` (YAML/JSON)/`None` (Python) to disable this behavior. Defaults to `60`. :::{vconf} server @@ -325,7 +325,7 @@ URL of your Vault installation. Required. ::: #### verify Configures certificate verification behavior when issuing requests to the -Vault server. If unset, requests will use the CA certificates bundled with `certifi`. +Vault server. If unset, requests use the CA certificates bundled with `certifi`. For details, please see the `requests` documentation on [certificate verification][]. @@ -336,7 +336,7 @@ sole trust anchor for certificate chain verification. :::{hint} This value can be set inside the minion configuration as well, from where it -will take precedence. +takes precedence. ::: :::{vconf} server:namespace @@ -379,7 +379,7 @@ params [Vault AppRole API docs][] for details. If you update these params, you can update the minion AppRoles manually using the runner [vault.sync_approles](saltext.vault.runners.vault.sync_approles), - but they will be updated automatically during a request by a minion as well. + but they are updated automatically during a request by a minion as well. :::{vconf} issue:token ::: @@ -390,7 +390,7 @@ Configuration regarding issued tokens. ::: role_name : Specifies the [Token Role][] name to use for creating minion tokens. - If omitted, minion tokens will be created without any role, thus being able + If omitted, minion tokens are created without any role, thus being able to inherit any master token policy (including token creation capabilities). :::{vconf} issue:token:params @@ -431,23 +431,39 @@ List of keys to use to unseal the Vault server with the Configures metadata for the issued entities/secrets. Values can be strings or [string templates](#vault-templating). -:::{note} -Values have to be strings, hence templated variables that resolve to lists -will be concatenated to a lexicographically sorted comma-separated list +:::{important} +Vault allows string-valued metadata only, hence templates that resolve to lists +are concatenated into a lexicographically sorted comma-separated list (Python `list.sort()`). + +In addition, each list item is rendered into a distinct key of its own, +determined by its parent key name and its list index. For example, if you +are templating a metadata key named `roles` using a list, which in this example +resolves to `[web, db]`, the single `roles` definition creates three +separate metadata entries: + +- `roles`: `db,web` +- `roles__0`: `db` +- `roles__1`: `web` + +For details on this, see the [templating guide](metadata-templating-target). ::: :::{vconf} metadata:entity ::: #### entity -Configures the metadata associated with the minion entity inside Vault. +Configures custom metadata Vault associates with the minion entity, +which can be referenced in [templated ACL policies](https://developer.hashicorp.com/vault/tutorials/policies/policy-templating). + +:::{important} Entities are only created when issuing AppRoles to minions. +::: :::{vconf} metadata:secret ::: #### secret -Configures the metadata associated with issued tokens/SecretIDs. They -are logged in plaintext to the Vault audit log. +Configures custom metadata associated with issued tokens/SecretIDs. +It is logged in plaintext to the Vault audit log. :::{vconf} policies ::: @@ -475,7 +491,7 @@ policies are written to Vault configuration the first time authentication data is requested (they can be refreshed on demand by running the [vault.sync_approles](saltext.vault.runners.vault.sync_approles) runner). -They will also be refreshed in case other {vconf}`issuance parameters ` are changed, either on the master or the minion +They are also refreshed in case other {vconf}`issuance parameters ` are changed, either on the master or the minion (if {vconf}`allow_minion_override_params` is True). ::: @@ -504,14 +520,14 @@ Hardcoded to True when issuing AppRoles. Using cached pillar data only (refresh_pillar=False) might cause the policies to be out of sync. If there is no cached pillar data available for the minion, -pillar templates will fail to render at all. +pillar templates fail to render at all. If you use pillar values for templating policies and do not disable refreshing pillar data, make sure the relevant values are not sourced from Vault (ext_pillar, sdb) or from a pillar sls file that uses the vault -execution/sdb module. Although this will often work when cached pillar data is +execution/sdb module. Although this often works when cached pillar data is available, if the master needs to compile the pillar data during policy rendering, -all Vault modules will be broken to prevent an infinite loop. +all Vault modules are broken to prevent an infinite loop. ::: ## Minion-only configuration @@ -525,10 +541,10 @@ and {vconf}`client` can be set on the minion as well, even if it pulls its confi ::: ### `config_location` Override the source of the Vault configuration for the minion. -By default, this extension will try to determine if it needs to request +By default, this extension tries to determine if it needs to request the connection details from the master or from the local config, depending on the minion running in local or master-connected mode. This option -will force the extension to use the connection details from the master or the +forces the extension to use the connection details from the master or the local config, regardless of circumstances. Allowed values: `master`, `local`. :::{vconf} issue_params diff --git a/docs/topics/templating.md b/docs/topics/templating.md index c6d000fb..ab825046 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,15 +82,79 @@ roles_map: ``` A pattern of `salt_role_{pillar[roles]}` (or `salt_role_{pillar[roles_map]}`) -will be expanded to: +is expanded into: ```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. +The way in which this expanded result is handled depends on the scope of the template. + +#### Policies/pillar paths +Policies and pillar paths treat the values separately, as intended. + +(metadata-templating-target)= +#### Entity/authentication metadata +Entity and authentication metadata is written to Vault, which +[only supports strings](https://github.com/hashicorp/vault/issues/12039). +Consequently, it's impossible to apply [templated ACL policies](https://developer.hashicorp.com/vault/tutorials/policies/policy-templating) for composite values like lists +(typically minion roles) the same way as for simple ones (e.g. minion ID). +A general solution to this involves templating {vconf}`policies:assign` and creating regular policies instead. + +:::{versionadded} 1.3.0 +::: +This extension supports an inelegant workaround though, +which allows to rely on templated ACL policies even when composite values are in play. + +Assuming the following master configuration: + +```yaml +vault: + metadata: + entity: + roles: '{pillar[roles]}' +``` + +A minion's metadata is rendered as follows: + +* The configured metadata key (`roles`) contains all values concatenated into a + lexicographically sorted comma-separated list. +* Furthermore, each value in this list implicitly receives its own metadata key, whose name + is derived by concatenating the configured key, a double underscore and the list item's index. + +Thus, a minion with `pillar[roles]` == `[web, db]` effectively receives the following metadata: + +```yaml +roles: db,web +roles__0: db +roles__1: web +``` + +To make use of this in a templated ACL policy, repeat each definition for as many times +as the number of items you want to support for the composite template variable, +only changing the metadata key index: + +```{code-block} vaultpolicy +:caption: This policy respects at most 3 different roles per minion + +# Resolves to salt/data/roles/db +path "salt/data/roles/{{identity.entity.metadata.roles__0}}" { + capabilities = ["read"] +} +# Resolves to salt/data/roles/web +path "salt/data/roles/{{identity.entity.metadata.roles__1}}" { + capabilities = ["read"] +} +# Ignored in this example +path "salt/data/roles/{{identity.entity.metadata.roles__2}}" { + capabilities = ["read"] +} +``` + +:::{note} +Since this expansion of a single user-defined key into an arbitrary +number of generated ones can theoretically lead to conflicts, it is skipped entirely +if any user-defined key begins with the designated prefix +(`roles__` in the above example). +::: diff --git a/src/saltext/vault/runners/vault.py b/src/saltext/vault/runners/vault.py index 5137e31f..378ea72a 100644 --- a/src/saltext/vault/runners/vault.py +++ b/src/saltext/vault/runners/vault.py @@ -1035,19 +1035,60 @@ 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 was 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 needs to account for this 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..e87b5c25 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 @@ -626,6 +638,7 @@ def vault_master_config(self, pillar_state_tree, vault_port): "ext_pillar": [ {"vault": "salt/minions/{minion}"}, {"vault": "salt/roles/{pillar[role]}"}, + {"vault": "salt/roles/{pillar[roles]}"}, ], "peer_run": { ".*": [ @@ -656,6 +669,7 @@ def vault_master_config(self, pillar_state_tree, vault_port): "entity": { "minion-id": "{minion}", "role": "{pillar[role]}", + "roles": "{pillar[roles]}", }, }, "policies": { @@ -733,6 +747,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")