Ensure unwrap client uses client config if available

changelog/95.fixed.md

@@ -0,0 +1 @@
+Fixed the client used for unwrapping authentication credentials not respecting `client` configuration when no cached configuration is available

src/saltext/vault/utils/vault/factory.py

@@ -500,8 +500,14 @@ def _get_connection_config(cbank, opts, context, force_local=False, pre_flush=Fa
         config_cache.flush(cbank=False)
 
     config_cache.store(new_config)
-    if unwrap_client is None:
-        unwrap_client = vclient.VaultClient(**new_config["server"], **new_config["client"])
+    # Always reinitialize client in case the unwrap_client `client` config is
+    # out of sync with the new config. This is a theoretical precaution since
+    # the client should be instantiated in _query_master using the new config.
+    unwrap_client = vclient.VaultClient(
+        **new_config["server"],
+        **new_config["client"],
+        session=unwrap_client.session if unwrap_client is not None else None,
+    )
     return new_config, embedded_token, unwrap_client
 
 
@@ -710,7 +716,7 @@ def check_result(
 
         if result.get("wrap_info") or result.get("wrap_info_nested"):
             if unwrap_client is None:
-                unwrap_client = vclient.VaultClient(**result["server"])
+                unwrap_client = vclient.VaultClient(**result["server"], **result.get("client", {}))
 
             for key in [""] + result.get("wrap_info_nested", []):
                 if key:

tests/unit/pillar/test_vault.py

@@ -101,9 +101,7 @@ def test_get_paths(pattern, expected):
     previous_pillar = {
         "role": "foo",
     }
-    result = vault._get_paths(  # pylint: disable=protected-access
-        pattern, "test-minion", previous_pillar
-    )
+    result = vault._get_paths(pattern, "test-minion", previous_pillar)
     assert result == expected
 
 

tests/unit/runners/vault/test_vault.py

@@ -370,14 +370,12 @@ def test_generate_token(
     token_serialized,
     wrapped_serialized,
     metadata_secret_default,
-):  # pylint: disable-msg=too-many-arguments
+):
     """
     Ensure _generate_token calls the API as expected
     """
     wrap = config("issue:wrap")
-    res_token, res_num_uses = vault._generate_token(  # pylint: disable=protected-access
-        "test-minion", issue_params=None, wrap=wrap
-    )
+    res_token, res_num_uses = vault._generate_token("test-minion", issue_params=None, wrap=wrap)
     endpoint = "auth/token/create"
     role_name = config("issue:token:role_name")
     payload = {}
@@ -407,17 +405,13 @@ def test_generate_token_no_policies_denied():
     Ensure generated tokens need at least one attached policy
     """
     with pytest.raises(salt.exceptions.SaltRunnerError, match=".*No policies matched minion.*"):
-        vault._generate_token(  # pylint: disable=protected-access
-            "test-minion", issue_params=None, wrap=False
-        )
+        vault._generate_token("test-minion", issue_params=None, wrap=False)
 
 
 @pytest.mark.parametrize("ttl", [None, 1337])
 @pytest.mark.parametrize("uses", [None, 1, 30])
 @pytest.mark.parametrize("config", [{}, {"issue:type": "approle"}], indirect=True)
-def test_generate_token_deprecated(
-    ttl, uses, token_serialized, config, validate_signature, caplog
-):  # pylint: disable-msg=too-many-arguments
+def test_generate_token_deprecated(ttl, uses, token_serialized, config, validate_signature, caplog):
     """
     Ensure the deprecated generate_token function returns data in the old format
     """
@@ -715,15 +709,13 @@ def test_get_role_id(
         manage_entity,
         manage_entity_alias,
         wrapped_serialized,
-    ):  # pylint: disable-msg=too-many-arguments
+    ):
         """
         Ensure _get_role_id returns data in the expected format and does not
         try to generate a new AppRole if it exists and is configured correctly
         """
         wrap = config("issue:wrap")
-        res = vault._get_role_id(  # pylint: disable=protected-access
-            "test-minion", issue_params=None, wrap=wrap
-        )
+        res = vault._get_role_id("test-minion", issue_params=None, wrap=wrap)
         lookup_approle.assert_called_with("test-minion")
         lookup_roleid.assert_called_with("test-minion", wrap=wrap)
         manage_approle.assert_not_called()
@@ -750,33 +742,32 @@ def test_get_role_id(
     )
     def test_get_role_id_generate_new(
         self,
-        config,  # pylint: disable=unused-argument
+        config,
         lookup_approle,
         lookup_roleid,
         manage_approle,
         manage_entity,
         manage_entity_alias,
         wrapped_serialized,
         issue_params,
-    ):  # pylint: disable-msg=too-many-arguments
+    ):
         """
         Ensure _get_role_id returns data in the expected format and does not
         try to generate a new AppRole if it exists and is configured correctly
         """
         lookup_approle.return_value = False
         wrap = config("issue:wrap")
-        res = vault._get_role_id(  # pylint: disable=protected-access
-            "test-minion", issue_params=issue_params, wrap=wrap
-        )
+        res = vault._get_role_id("test-minion", issue_params=issue_params, wrap=wrap)
         assert res == wrapped_serialized
         lookup_roleid.assert_called_with("test-minion", wrap=wrap)
         manage_approle.assert_called_once_with("test-minion", issue_params)
         manage_entity.assert_called_once_with("test-minion")
         manage_entity_alias.assert_called_once_with("test-minion")
 
+    @pytest.mark.usefixtures("config")
     @pytest.mark.parametrize("config", [{"issue:type": "approle"}], indirect=True)
     def test_get_role_id_generate_new_errors_on_generation_failure(
-        self, config, lookup_approle, lookup_roleid  # pylint: disable=unused-argument
+        self, lookup_approle, lookup_roleid
     ):
         """
         Ensure _get_role_id returns an error if the AppRole generation failed
@@ -787,9 +778,7 @@ def test_get_role_id_generate_new_errors_on_generation_failure(
             salt.exceptions.SaltRunnerError,
             match="Failed to create AppRole for minion.*",
         ):
-            vault._get_role_id(  # pylint: disable=protected-access
-                "test-minion", issue_params=None, wrap=False
-            )
+            vault._get_role_id("test-minion", issue_params=None, wrap=False)
 
 
 @pytest.mark.parametrize(
@@ -951,9 +940,7 @@ def test_get_policies(expected, grains, pillar):
             "saltext.vault.utils.vault.helpers.expand_pattern_lists",
             Mock(side_effect=lambda x, *args, **kwargs: [x]),
         ):
-            res = vault._get_policies(  # pylint: disable=protected-access
-                "test-minion", refresh_pillar=False
-            )
+            res = vault._get_policies("test-minion", refresh_pillar=False)
             assert res == expected
 
 
@@ -979,9 +966,7 @@ def test_get_policies_does_not_render_pillar_unnecessarily(config, grains, pilla
         ):
             with patch("salt.pillar.get_pillar", autospec=True) as get_pillar:
                 get_pillar.return_value.compile_pillar.return_value = pillar
-                vault._get_policies(  # pylint: disable=protected-access
-                    "test-minion", refresh_pillar=True
-                )
+                vault._get_policies("test-minion", refresh_pillar=True)
                 assert get_pillar.call_count == int("pillar" in config("policies:assign")[0])
 
 
@@ -1005,9 +990,7 @@ def test_get_policies_for_nonexisting_minions(expected):
             "saltext.vault.utils.vault.helpers.expand_pattern_lists",
             Mock(side_effect=lambda x, *args, **kwargs: [x]),
         ):
-            res = vault._get_policies(  # pylint: disable=protected-access
-                "test-minion", refresh_pillar=False
-            )
+            res = vault._get_policies("test-minion", refresh_pillar=False)
             assert res == expected
 
 
@@ -1047,9 +1030,7 @@ def test_get_metadata(metadata_patterns, expected, pillar):
             "saltext.vault.utils.vault.helpers.expand_pattern_lists",
             Mock(side_effect=lambda x, *args, **kwargs: [x]),
         ):
-            res = vault._get_metadata(  # pylint: disable=protected-access
-                "test-minion", metadata_patterns, refresh_pillar=False
-            )
+            res = vault._get_metadata("test-minion", metadata_patterns, refresh_pillar=False)
             assert res == expected
 
 
@@ -1065,7 +1046,7 @@ def test_get_metadata_list():
             "saltext.vault.utils.vault.helpers.expand_pattern_lists", autospec=True
         ) as expand:
             expand.return_value = ["salt_role_foo", "salt_role_bar"]
-            res = vault._get_metadata(  # pylint: disable=protected-access
+            res = vault._get_metadata(
                 "test-minion",
                 {"salt_role": "salt_role_{pillar[roles]}"},
                 refresh_pillar=False,
@@ -1277,7 +1258,7 @@ def test_parse_issue_params(issue_params, expected):
     Ensure all known parameters can only be overridden if it was configured
     on the master. Also ensure the mapping to API requests is correct (for tokens).
     """
-    res = vault._parse_issue_params(issue_params)  # pylint: disable=protected-access
+    res = vault._parse_issue_params(issue_params)
     assert res == expected
 
 
@@ -1315,7 +1296,7 @@ def test_parse_issue_params_does_not_allow_bind_secret_id_override(issue_params,
     """
     Ensure bind_secret_id can only be set on the master.
     """
-    res = vault._parse_issue_params(issue_params)  # pylint: disable=protected-access
+    res = vault._parse_issue_params(issue_params)
     assert res.get("bind_secret_id", False) == expected
 
 
@@ -1324,7 +1305,7 @@ def test_manage_approle(approle_api, policies_default):
     """
     Ensure _manage_approle calls the API as expected.
     """
-    vault._manage_approle("test-minion", None)  # pylint: disable=protected-access
+    vault._manage_approle("test-minion", None)
     approle_api.write_approle.assert_called_once_with(
         "test-minion",
         mount="salt-minions",
@@ -1339,7 +1320,7 @@ def test_delete_approle(approle_api):
     """
     Ensure _delete_approle calls the API as expected.
     """
-    vault._delete_approle("test-minion")  # pylint: disable=protected-access
+    vault._delete_approle("test-minion")
     approle_api.delete_approle.assert_called_once_with("test-minion", mount="salt-minions")
 
 
@@ -1349,7 +1330,7 @@ def test_lookup_approle(approle_api, approle_meta):
     Ensure _lookup_approle calls the API as expected.
     """
     approle_api.read_approle.return_value = approle_meta
-    res = vault._lookup_approle("test-minion")  # pylint: disable=protected-access
+    res = vault._lookup_approle("test-minion")
     assert res == approle_meta
     approle_api.read_approle.assert_called_once_with("test-minion", mount="salt-minions")
 
@@ -1360,7 +1341,7 @@ def test_lookup_approle_nonexistent(approle_api):
     Ensure _lookup_approle catches VaultNotFoundErrors and returns False.
     """
     approle_api.read_approle.side_effect = vaultutil.VaultNotFoundError
-    res = vault._lookup_approle("test-minion")  # pylint: disable=protected-access
+    res = vault._lookup_approle("test-minion")
     assert res is False
 
 
@@ -1370,7 +1351,7 @@ def test_lookup_role_id(approle_api, wrap):
     """
     Ensure _lookup_role_id calls the API as expected.
     """
-    vault._lookup_role_id("test-minion", wrap=wrap)  # pylint: disable=protected-access
+    vault._lookup_role_id("test-minion", wrap=wrap)
     approle_api.read_role_id.assert_called_once_with("test-minion", mount="salt-minions", wrap=wrap)
 
 
@@ -1380,7 +1361,7 @@ def test_lookup_role_id_nonexistent(approle_api):
     Ensure _lookup_role_id catches VaultNotFoundErrors and returns False.
     """
     approle_api.read_role_id.side_effect = vaultutil.VaultNotFoundError
-    res = vault._lookup_role_id("test-minion", wrap=False)  # pylint: disable=protected-access
+    res = vault._lookup_role_id("test-minion", wrap=False)
     assert res is False
 
 
@@ -1390,7 +1371,7 @@ def test_get_secret_id(approle_api, wrap):
     """
     Ensure _get_secret_id calls the API as expected.
     """
-    vault._get_secret_id("test-minion", wrap=wrap)  # pylint: disable=protected-access
+    vault._get_secret_id("test-minion", wrap=wrap)
     approle_api.generate_secret_id.assert_called_once_with(
         "test-minion",
         metadata=ANY,
@@ -1405,7 +1386,7 @@ def test_lookup_entity_by_alias(identity_api):
     Ensure _lookup_entity_by_alias calls the API as expected.
     """
     with patch("saltext.vault.runners.vault._lookup_role_id", return_value="test-role-id"):
-        vault._lookup_entity_by_alias("test-minion")  # pylint: disable=protected-access
+        vault._lookup_entity_by_alias("test-minion")
         identity_api.read_entity_by_alias.assert_called_once_with(
             alias="test-role-id", mount="salt-minions"
         )
@@ -1418,7 +1399,7 @@ def test_lookup_entity_by_alias_failed(identity_api):
     """
     with patch("saltext.vault.runners.vault._lookup_role_id", return_value="test-role-id"):
         identity_api.read_entity_by_alias.side_effect = vaultutil.VaultNotFoundError
-        res = vault._lookup_entity_by_alias("test-minion")  # pylint: disable=protected-access
+        res = vault._lookup_entity_by_alias("test-minion")
         assert res is False
 
 
@@ -1427,7 +1408,7 @@ def test_fetch_entity_by_name(identity_api):
     """
     Ensure _fetch_entity_by_name calls the API as expected.
     """
-    vault._fetch_entity_by_name("test-minion")  # pylint: disable=protected-access
+    vault._fetch_entity_by_name("test-minion")
     identity_api.read_entity.assert_called_once_with(name="salt_minion_test-minion")
 
 
@@ -1437,18 +1418,16 @@ def test_fetch_entity_by_name_failed(identity_api):
     Ensure _fetch_entity_by_name returns False if the lookup fails.
     """
     identity_api.read_entity.side_effect = vaultutil.VaultNotFoundError
-    res = vault._fetch_entity_by_name("test-minion")  # pylint: disable=protected-access
+    res = vault._fetch_entity_by_name("test-minion")
     assert res is False
 
 
-@pytest.mark.usefixtures("config")
-def test_manage_entity(
-    identity_api, metadata, metadata_entity_default
-):  # pylint: disable=unused-argument
+@pytest.mark.usefixtures("config", "metadata")
+def test_manage_entity(identity_api, metadata_entity_default):
     """
     Ensure _manage_entity calls the API as expected.
     """
-    vault._manage_entity("test-minion")  # pylint: disable=protected-access
+    vault._manage_entity("test-minion")
     identity_api.write_entity.assert_called_with(
         "salt_minion_test-minion", metadata=metadata_entity_default
     )
@@ -1459,7 +1438,7 @@ def test_delete_entity(identity_api):
     """
     Ensure _delete_entity calls the API as expected.
     """
-    vault._delete_entity("test-minion")  # pylint: disable=protected-access
+    vault._delete_entity("test-minion")
     identity_api.delete_entity.assert_called_with("salt_minion_test-minion")
 
 
@@ -1469,7 +1448,7 @@ def test_manage_entity_alias(identity_api):
     Ensure _manage_entity_alias calls the API as expected.
     """
     with patch("saltext.vault.runners.vault._lookup_role_id", return_value="test-role-id"):
-        vault._manage_entity_alias("test-minion")  # pylint: disable=protected-access
+        vault._manage_entity_alias("test-minion")
         identity_api.write_entity_alias.assert_called_with(
             "salt_minion_test-minion", alias_name="test-role-id", mount="salt-minions"
         )
@@ -1486,22 +1465,22 @@ def test_manage_entity_alias_raises_errors(identity_api):
             salt.exceptions.SaltRunnerError,
             match="Cannot create alias.* no entity found.",
         ):
-            vault._manage_entity_alias("test-minion")  # pylint: disable=protected-access
+            vault._manage_entity_alias("test-minion")
 
 
 def test_revoke_token_by_token(client):
     """
     Ensure _revoke_token calls the API as expected.
     """
-    vault._revoke_token(token="test-token")  # pylint: disable=protected-access
+    vault._revoke_token(token="test-token")
     client.post.assert_called_once_with("auth/token/revoke", payload={"token": "test-token"})
 
 
 def test_revoke_token_by_accessor(client):
     """
     Ensure _revoke_token calls the API as expected.
     """
-    vault._revoke_token(accessor="test-accessor")  # pylint: disable=protected-access
+    vault._revoke_token(accessor="test-accessor")
     client.post.assert_called_once_with(
         "auth/token/revoke-accessor", payload={"accessor": "test-accessor"}
     )