Merge 3006.x into 3007.x

changelog/65450.fixed.md

@@ -0,0 +1 @@
+Fix regression of fileclient re-use when rendering sls pillars and states

changelog/66041.fixed.md

@@ -0,0 +1 @@
+Use hmac compare_digest method in hashutil module to mitigate potential timing attacks

salt/loader/__init__.py

@@ -20,7 +20,6 @@ import salt.syspaths
 import salt.utils.context
 import salt.utils.data
 import salt.utils.dictupdate
-import salt.utils.event
 import salt.utils.files
 import salt.utils.lazy
 import salt.utils.odict
@@ -358,6 +357,8 @@ def minion_mods(
                         ret[f_key] = funcs[func]
 
     if notify:
+        import salt.utils.event
+
         with salt.utils.event.get_event("minion", opts=opts, listen=False) as evt:
             evt.fire_event(
                 {"complete": True}, tag=salt.defaults.events.MINION_MOD_REFRESH_COMPLETE
@@ -534,6 +535,7 @@ def utils(
     whitelist=None,
     context=None,
     proxy=None,
+    file_client=None,
     pack_self=None,
     loaded_base_name=None,
 ):
@@ -553,7 +555,11 @@ def utils(
         opts,
         tag="utils",
         whitelist=whitelist,
-        pack={"__context__": context, "__proxy__": proxy or {}},
+        pack={
+            "__context__": context,
+            "__proxy__": proxy or {},
+            "__file_client__": file_client,
+        },
         pack_self=pack_self,
         loaded_base_name=loaded_base_name,
         _only_pack_properly_namespaced_functions=False,
@@ -908,7 +914,13 @@ def ssh_wrapper(opts, functions=None, context=None, loaded_base_name=None):
 
 
 def render(
-    opts, functions, states=None, proxy=None, context=None, loaded_base_name=None
+    opts,
+    functions,
+    states=None,
+    proxy=None,
+    context=None,
+    file_client=None,
+    loaded_base_name=None,
 ):
     """
     Returns the render modules
@@ -929,6 +941,7 @@ def render(
         "__salt__": functions,
         "__grains__": opts.get("grains", {}),
         "__context__": context,
+        "__file_client__": file_client,
     }
 
     if states:

salt/loader/dunder.py

@@ -6,4 +6,4 @@ import salt.loader.context
 loader_context = salt.loader.context.LoaderContext()
 
 
-__file_client__ = loader_context.named_context("__file_client__")
+__file_client__ = loader_context.named_context("__file_client__", default=None)

salt/modules/hashutil.py

@@ -294,4 +294,4 @@ def github_signature(string, shared_secret, challenge_hmac):
     if isinstance(key, str):
         key = salt.utils.stringutils.to_bytes(key)
     hmac_hash = hmac.new(key, msg, getattr(hashlib, hashtype))
-    return hmac_hash.hexdigest() == challenge
+    return hmac.compare_digest(hmac_hash.hexdigest(), challenge)

salt/payload.py

@@ -9,7 +9,6 @@ import datetime
 import gc
 import logging
 
-import salt.loader.context
 import salt.transport.frame
 import salt.utils.immutabletypes as immutabletypes
 import salt.utils.msgpack

salt/pillar/__init__.py

@@ -571,17 +571,23 @@ class Pillar:
 
         # if we didn't pass in functions, lets load them
         if functions is None:
-            utils = salt.loader.utils(opts)
+            utils = salt.loader.utils(opts, file_client=self.client)
             if opts.get("file_client", "") == "local":
-                self.functions = salt.loader.minion_mods(opts, utils=utils)
+                self.functions = salt.loader.minion_mods(
+                    opts, utils=utils, file_client=self.client
+                )
             else:
-                self.functions = salt.loader.minion_mods(self.opts, utils=utils)
+                self.functions = salt.loader.minion_mods(
+                    self.opts, utils=utils, file_client=self.client
+                )
         else:
             self.functions = functions
 
         self.opts["minion_id"] = minion_id
         self.matchers = salt.loader.matchers(self.opts)
-        self.rend = salt.loader.render(self.opts, self.functions)
+        self.rend = salt.loader.render(
+            self.opts, self.functions, self.client, file_client=self.client
+        )
         ext_pillar_opts = copy.deepcopy(self.opts)
         # Keep the incoming opts ID intact, ie, the master id
         if "id" in opts:

salt/state.py

@@ -1299,7 +1299,7 @@ class State:
         Load the modules into the state
         """
         log.info("Loading fresh modules for state activity")
-        self.utils = salt.loader.utils(self.opts)
+        self.utils = salt.loader.utils(self.opts, file_client=self.file_client)
         self.functions = salt.loader.minion_mods(
             self.opts,
             self.state_con,
@@ -1331,6 +1331,7 @@ class State:
             self.functions,
             states=self.states,
             proxy=self.proxy,
+            file_client=self.file_client,
             context=self.state_con,
         )
 

salt/utils/crypt.py

@@ -5,7 +5,6 @@ import hashlib
 import logging
 import os
 
-import salt.loader
 import salt.utils.files
 from salt.exceptions import SaltInvocationError
 
@@ -104,6 +103,10 @@ def decrypt(
     if renderers is None:
         if opts is None:
             raise TypeError("opts are required")
+
+        # Avaoid circular import
+        import salt.loader
+
         renderers = salt.loader.render(opts, {})
 
     rend_func = renderers.get(rend)

salt/utils/templates.py

@@ -29,6 +29,7 @@ import salt.utils.yamlencoding
 from salt import __path__ as saltpath
 from salt.exceptions import CommandExecutionError, SaltInvocationError, SaltRenderError
 from salt.loader.context import NamedLoaderContext
+from salt.loader.dunder import __file_client__
 from salt.utils.decorators.jinja import JinjaFilter, JinjaGlobal, JinjaTest
 from salt.utils.odict import OrderedDict
 from salt.utils.versions import Version
@@ -342,7 +343,6 @@ def render_jinja_tmpl(tmplstr, context, tmplpath=None):
     saltenv = context["saltenv"]
     loader = None
     newline = False
-    file_client = context.get("fileclient", None)
 
     if tmplstr and not isinstance(tmplstr, str):
         # https://jinja.palletsprojects.com/en/2.11.x/api/#unicode
@@ -362,7 +362,7 @@ def render_jinja_tmpl(tmplstr, context, tmplpath=None):
                 opts,
                 saltenv,
                 pillar_rend=context.get("_pillar_rend", False),
-                _file_client=file_client,
+                _file_client=context.get("fileclient", __file_client__.value()),
             )
 
         env_args = {"extensions": [], "loader": loader}

tests/conftest.py

@@ -473,7 +473,7 @@ def pytest_collection_modifyitems(config, items):
             ):
                 # Let's apply the timeout marker on the test, if the marker
                 # is not already applied
-                item.add_marker(pytest.mark.timeout(60))
+                item.add_marker(pytest.mark.timeout(90))
         for fixture in item.fixturenames:
             if fixture not in item._fixtureinfo.name2fixturedefs:
                 continue

tests/pytests/functional/test_fileclient_reuse.py

@@ -0,0 +1,114 @@
+import pytest
+
+import salt.loader
+import salt.pillar
+import salt.state
+import salt.utils.cache
+import salt.utils.jinja
+from tests.support.mock import patch
+
+
+@pytest.fixture
+def mock_cached_loader():
+    """
+    Mock the SaltCacheLoader
+
+    The mock keeps track of the number of
+    instantiations and the most recent args and kwargs.
+    """
+
+    class CacheLoader(salt.utils.jinja.SaltCacheLoader):
+        args = []
+        kwargs = {}
+        calls = 0
+
+        def __init__(self, *args, **kwargs):
+            self.__class__.calls += 1
+            self.__class__.args = args
+            self.__class__.kwargs = kwargs
+            super().__init__(*args, **kwargs)
+
+    with patch("salt.utils.jinja.SaltCacheLoader", CacheLoader):
+        yield CacheLoader
+
+
+def test_pillar_tops(temp_salt_master, temp_salt_minion, tmp_path, mock_cached_loader):
+    """
+    pillar fileclient is used while rendering pillar tops
+    """
+    tops = """
+    base:
+      "*":
+        - test_pillar
+    """
+    pillarsls = """
+    foo: bar
+    """
+    opts = temp_salt_master.config.copy()
+
+    with temp_salt_master.pillar_tree.base.temp_file("top.sls", tops):
+        with temp_salt_master.pillar_tree.base.temp_file("test_pillar.sls", pillarsls):
+            grains = salt.loader.grains(opts)
+            pillar = salt.pillar.Pillar(
+                opts,
+                grains,
+                temp_salt_minion.id,
+                "base",
+            )
+            pillar.get_tops()
+            assert mock_cached_loader.calls == 1
+            assert "_file_client" in mock_cached_loader.kwargs
+            assert mock_cached_loader.kwargs["_file_client"] == pillar.client
+
+
+def test_pillar_render(
+    temp_salt_master, temp_salt_minion, tmp_path, mock_cached_loader
+):
+    """
+    pillar fileclient is used while rendering jinja pillar
+    """
+    tops = """
+    base:
+      "*":
+        - test_pillar
+    """
+    pillarsls = """
+    foo: bar
+    """
+    opts = temp_salt_master.config.copy()
+
+    with temp_salt_master.pillar_tree.base.temp_file("top.sls", tops):
+        with temp_salt_master.pillar_tree.base.temp_file("test_pillar.sls", pillarsls):
+            grains = salt.loader.grains(opts)
+            pillar = salt.pillar.Pillar(
+                opts,
+                grains,
+                temp_salt_minion.id,
+                "base",
+            )
+            pdata = pillar.render_pillar({"base": ["test_pillar"]})
+            assert pdata == ({"foo": "bar"}, [])
+            assert mock_cached_loader.calls == 1
+            assert "_file_client" in mock_cached_loader.kwargs
+            assert mock_cached_loader.kwargs["_file_client"] == pillar.client
+
+
+def test_highstate(temp_salt_master, temp_salt_minion, tmp_path, mock_cached_loader):
+    """
+    pillar fileclient is used while rendering pillar tops
+    """
+    statesls = """
+    test_state:
+      cmd.run:
+        - name: echos foo
+    """
+    opts = temp_salt_master.config.copy()
+
+    with temp_salt_master.state_tree.base.temp_file("test_state.sls", statesls):
+        highstate = salt.state.HighState(
+            opts,
+        )
+        a = highstate.render_highstate({"base": ["test_state"]})
+        assert mock_cached_loader.calls == 1
+        assert "_file_client" in mock_cached_loader.kwargs
+        assert mock_cached_loader.kwargs["_file_client"] == highstate.client

tests/pytests/integration/cli/test_salt_deltaproxy.py

@@ -581,7 +581,6 @@ def ping():
     [True, False],
     ids=["parallel_startup=True", "parallel_startup=False"],
 )
-@pytest.mark.timeout_unless_on_windows(120)
 def test_custom_proxy_module_raise_exception(
     salt_master,
     salt_cli,
@@ -712,7 +711,6 @@ def ping():
 # Hangs on Windows. You can add a timeout to the proxy.run command, but then
 # it just times out.
 @pytest.mark.skip_on_windows(reason=PRE_PYTEST_SKIP_REASON)
-@pytest.mark.timeout_unless_on_windows(180)
 @pytest.mark.parametrize(
     "parallel_startup",
     [True, False],

tests/pytests/unit/cloud/test_cloud.py

@@ -4,6 +4,10 @@ from salt.cloud import Cloud
 from salt.exceptions import SaltCloudSystemExit
 from tests.support.mock import MagicMock, patch
 
+pytestmark = [
+    pytest.mark.timeout_unless_on_windows(120),
+]
+
 
 @pytest.fixture
 def master_config(master_opts):

tests/pytests/unit/modules/test_hashutil.py

@@ -6,6 +6,7 @@
 import pytest
 
 import salt.modules.hashutil as hashutil
+from tests.support.mock import patch
 
 
 @pytest.fixture
@@ -84,3 +85,9 @@ def test_hmac_compute(the_string, the_string_hmac_compute):
 
 def test_github_signature(the_string, the_string_github):
     assert hashutil.github_signature(the_string, "shared secret", the_string_github)
+
+
+def test_github_signature_uses_hmac_compare_digest(the_string, the_string_github):
+    with patch("hmac.compare_digest") as hmac_compare:
+        assert hashutil.github_signature(the_string, "shared secret", the_string_github)
+        hmac_compare.assert_called_once()

tests/unit/utils/test_process.py

@@ -26,6 +26,10 @@ except ImportError:
 
 log = logging.getLogger(__name__)
 
+pytestmark = [
+    pytest.mark.timeout_unless_on_windows(120),
+]
+
 
 def die(func):
     """