From b6b10f8b6a231c2cab0a15924b2197081773f329 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 3 Aug 2026 17:28:05 -0700 Subject: [PATCH 1/2] Cache RSA verifier/signer bridge objects and mtime-key PublicKey.from_file Under sustained load a busy MWorker rebuilds cryptography + libcrypto RSA state on every public-key operation. memray on a stressed 3008.x master showed ~5,000 RSAX931Verifier.__init__ calls per 60 seconds against a matching PublicKey.decrypt call count. Same pattern on the sign side. Three layers of caching: 1. Lazy per-instance _verifier / _signer on PublicKey / PrivateKey. self.key is immutable after __init__, so the derived libcrypto bridge object can be reused for the lifetime of the instance. 2. Path-level cache on PublicKey.from_file keyed on (path, mtime). A key rotation on disk bumps mtime and invalidates the cache automatically. 3. Retry-on-verify-fail in PublicKey.verify / .decrypt. Preserves the pre-cache "always fresh" behavior for edge cases where a rotation preserves mtime (cp -p, NFS mtime cache, atomic rename with preserved timestamps). On the first failure the cache entry is evicted and one reload-and-retry is attempted. Genuine bad signatures still return False / raise ValueError; the retry costs one extra file read + PEM parse per forged attempt. Fixes #69940 --- changelog/69940.fixed.md | 8 ++ salt/crypt.py | 132 +++++++++++++++++-- tests/pytests/unit/test_crypt.py | 220 +++++++++++++++++++++++++++++++ 3 files changed, 347 insertions(+), 13 deletions(-) create mode 100644 changelog/69940.fixed.md diff --git a/changelog/69940.fixed.md b/changelog/69940.fixed.md new file mode 100644 index 000000000000..5b9caa05ef8a --- /dev/null +++ b/changelog/69940.fixed.md @@ -0,0 +1,8 @@ +Cache the libcrypto-backed RSAX931 verifier / signer objects on +``salt.crypt.PublicKey`` and ``PrivateKey`` instances and route +``PublicKey.from_file`` through an mtime-keyed path cache. Eliminates +thousands of redundant PEM parses and libcrypto ``BIO``/``RSA`` allocations +per minute in a busy master's ``MWorker`` processes. ``PublicKey.verify`` +and ``PublicKey.decrypt`` fall back to a one-shot reload-and-retry when a +cached key doesn't validate, preserving the pre-cache behavior for on-disk +rotations that don't bump mtime. diff --git a/salt/crypt.py b/salt/crypt.py index bbdc7b2e248f..98dfb68e22bb 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -347,14 +347,22 @@ def __init__(self, key_bytes, passphrase=None): raise InvalidKeyError("Encountered bad RSA private key") except cryptography.exceptions.UnsupportedAlgorithm: raise InvalidKeyError("Unsupported key algorithm") + # Lazy cache of the libcrypto-backed X9.31 signer. ``self.key`` is + # immutable after __init__ so the derived signer can be reused for the + # lifetime of this instance. When PrivateKey instances are reused via + # the get_rsa_key path-level cache this eliminates repeated PEM + # serialization + libcrypto BIO/RSA allocation on every encrypt(). + self._signer = None def encrypt(self, data): - pem = self.key.private_bytes( - encoding=serialization.Encoding.PEM, - format=serialization.PrivateFormat.TraditionalOpenSSL, - encryption_algorithm=serialization.NoEncryption(), - ) - return salt.utils.rsax931.RSAX931Signer(pem).sign(data) + if self._signer is None: + pem = self.key.private_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PrivateFormat.TraditionalOpenSSL, + encryption_algorithm=serialization.NoEncryption(), + ) + self._signer = salt.utils.rsax931.RSAX931Signer(pem) + return self._signer.sign(data) def sign(self, data, algorithm=PKCS1v15_SHA1): _padding = self.parse_padding_for_signing(algorithm) @@ -397,6 +405,18 @@ def public_key(self): class PublicKey(BaseKey): + @classmethod + def from_file(cls, path, *args, **kwargs): + """ + Return a ``PublicKey`` for the on-disk public key at ``path``. + + Routes through the mtime-keyed cache so callers that repeatedly load + the same key file share a single ``PublicKey`` instance (and therefore + a single cached ``RSAX931Verifier``). A key rotation on disk bumps the + file's mtime and invalidates the cache automatically. + """ + return _get_pub_key_with_evict(path, str(os.path.getmtime(path))) + def __init__(self, key_bytes): log.debug("Loading public key") try: @@ -405,6 +425,12 @@ def __init__(self, key_bytes): raise InvalidKeyError("Encountered bad RSA public key") except cryptography.exceptions.UnsupportedAlgorithm: raise InvalidKeyError("Unsupported key algorithm") + # Lazy cache of the libcrypto-backed X9.31 verifier. ``self.key`` is + # immutable after __init__ so the derived verifier can be reused for + # the lifetime of this instance. When PublicKey instances are reused + # via the from_file() path-level cache this eliminates repeated PEM + # serialization + libcrypto BIO/RSA allocation on every decrypt(). + self._verifier = None def encrypt(self, data, algorithm=OAEP_SHA1): _padding = self.parse_padding_for_encryption(algorithm) @@ -426,7 +452,7 @@ def encrypt(self, data, algorithm=OAEP_SHA1): except cryptography.exceptions.UnsupportedAlgorithm: raise UnsupportedAlgorithm(f"Unsupported algorithm: {algorithm}") - def verify(self, data, signature, algorithm=PKCS1v15_SHA1): + def _verify(self, data, signature, algorithm): _padding = self.parse_padding_for_signing(algorithm) _hash = self.parse_hash(algorithm) if SHA1 in algorithm and fips_enabled(): @@ -447,13 +473,41 @@ def verify(self, data, signature, algorithm=PKCS1v15_SHA1): return False return True + def verify(self, data, signature, algorithm=PKCS1v15_SHA1): + result = self._verify(data, signature, algorithm) + if result: + return True + # Preserve the pre-cache "always fresh" behavior for edge cases where + # a key rotated on disk without bumping mtime (cp -p, NFS mtime cache, + # atomic rename that preserves timestamps). If we own an entry in the + # public-key cache for this instance, evict it and retry once with a + # freshly loaded key. Genuine bad signatures still return False and + # only cost one extra file read + PEM parse per forged attempt. + fresh = _reload_evicted_pub_key(self) + if fresh is None or fresh is self: + return False + return fresh._verify(data, signature, algorithm) + + def _decrypt(self, data): + if self._verifier is None: + pem = self.key.public_bytes( + encoding=serialization.Encoding.PEM, + format=serialization.PublicFormat.SubjectPublicKeyInfo, + ) + self._verifier = salt.utils.rsax931.RSAX931Verifier(pem) + return self._verifier.verify(data) + def decrypt(self, data): - pem = self.key.public_bytes( - encoding=serialization.Encoding.PEM, - format=serialization.PublicFormat.SubjectPublicKeyInfo, - ) - verifier = salt.utils.rsax931.RSAX931Verifier(pem) - return verifier.verify(data) + try: + return self._decrypt(data) + except ValueError: + # X9.31 verify failed. Mirror verify()'s retry-on-fail semantics + # so a rotated-on-disk key without an mtime bump doesn't wedge a + # cached instance. Genuine bad payloads re-raise after retry. + fresh = _reload_evicted_pub_key(self) + if fresh is None or fresh is self: + raise + return fresh._decrypt(data) class PrivateKeyString(PrivateKey): @@ -463,6 +517,7 @@ def __init__(self, data, password=None): data.encode(), password=password, ) + self._signer = None # pylint: enable=super-init-not-called @@ -474,6 +529,7 @@ def __init__(self, data): self.key = serialization.load_pem_public_key(data.encode()) except ValueError: raise InvalidKeyError("Invalid key") + self._verifier = None # pylint: enable=super-init-not-called @@ -487,6 +543,56 @@ def get_rsa_key(path, passphrase): return PrivateKey.from_file(path, passphrase).key +# Path-level cache for PublicKey instances. Keyed on (path, mtime_str) so a +# rotation on disk (which bumps mtime) transparently loads a fresh instance. +# A parallel index (path -> current key) supports the retry-on-verify-fail +# eviction path in PublicKey.verify()/decrypt() for the corner cases where a +# key is replaced on disk without an mtime change (cp -p, NFS mtime cache, +# atomic rename with preserved timestamps). +_pub_key_cache = {} +_pub_key_cache_path_index = {} + + +def _get_pub_key_with_evict(path, timestamp): + """ + Load a ``PublicKey`` from disk, caching it by (path, mtime). + + ``timestamp`` should be the file's mtime as a string so a key rotation on + disk (which bumps mtime) invalidates the cache. Callers should route + through ``PublicKey.from_file`` rather than call this directly. + """ + cache_key = (path, timestamp) + cached = _pub_key_cache.get(cache_key) + if cached is not None: + return cached + with salt.utils.files.fopen(path, "rb") as fp: + pub = PublicKey(fp.read()) + _pub_key_cache[cache_key] = pub + _pub_key_cache_path_index[path] = cache_key + return pub + + +def _reload_evicted_pub_key(instance): + """ + Evict ``instance`` from the public-key cache and return a freshly loaded + ``PublicKey`` for the same path, or ``None`` if the instance isn't cached + or the underlying file is no longer readable. + + Used by ``PublicKey.verify``/``decrypt`` to preserve the pre-cache + "always fresh" behavior when a key rotates on disk without an mtime bump. + """ + for path, cache_key in list(_pub_key_cache_path_index.items()): + cached = _pub_key_cache.get(cache_key) + if cached is instance: + _pub_key_cache.pop(cache_key, None) + _pub_key_cache_path_index.pop(path, None) + try: + return _get_pub_key_with_evict(path, str(os.path.getmtime(path))) + except OSError: + return None + return None + + def get_rsa_pub_key(path): """ Return a public key from bytes diff --git a/tests/pytests/unit/test_crypt.py b/tests/pytests/unit/test_crypt.py index 691f8970491d..274e913d6282 100644 --- a/tests/pytests/unit/test_crypt.py +++ b/tests/pytests/unit/test_crypt.py @@ -645,3 +645,223 @@ async def mock_sign_in(*args, **kwargs): assert isinstance(auth._creds, dict) assert auth._creds["aes"] == aes assert auth._creds["session"] == session + + +# --- PublicKey / PrivateKey caching regression tests -------------------------- + + +@pytest.fixture +def _clear_pub_key_cache(): + """ + Clear the module-level public-key cache before and after each test so + tests can make hard assertions about cache membership and identity. + """ + crypt._pub_key_cache.clear() + crypt._pub_key_cache_path_index.clear() + yield + crypt._pub_key_cache.clear() + crypt._pub_key_cache_path_index.clear() + + +@pytest.fixture +def _rsa_keypair(tmp_path): + """ + Generate an RSA keypair once per test and write both halves to disk so + tests exercise ``PublicKey.from_file`` / ``PrivateKey.from_file``. + """ + priv_pem, pub_pem = crypt.gen_keys(2048) + priv_path = tmp_path / "test.pem" + pub_path = tmp_path / "test.pub" + priv_path.write_text(priv_pem) + pub_path.write_text(pub_pem) + return { + "priv_pem": priv_pem, + "pub_pem": pub_pem, + "priv_path": str(priv_path), + "pub_path": str(pub_path), + } + + +def _count_class_init(cls): + """ + Return a context-manager-like helper that instruments ``cls.__init__`` to + count the number of calls it receives. Returns a ``dict`` whose ``count`` + key holds the running total; caller is responsible for restoring the + original ``__init__`` when done. + """ + counter = {"count": 0, "original": cls.__init__} + + def wrapper(self, *args, **kwargs): + counter["count"] += 1 + return counter["original"](self, *args, **kwargs) + + cls.__init__ = wrapper + return counter + + +def test_publickey_verifier_cached_across_decrypts(_rsa_keypair): + """ + Repeated ``PublicKey.decrypt`` calls on a single instance must build the + underlying ``RSAX931Verifier`` exactly once. Pre-fix behavior was one + verifier per decrypt() call. + """ + import salt.utils.rsax931 + + priv = crypt.PrivateKey.from_str(_rsa_keypair["priv_pem"]) + pub = crypt.PublicKey.from_str(_rsa_keypair["pub_pem"]) + signed = priv.encrypt(b"salt") + + counter = _count_class_init(salt.utils.rsax931.RSAX931Verifier) + try: + for _ in range(50): + assert pub.decrypt(signed) == b"salt" + finally: + salt.utils.rsax931.RSAX931Verifier.__init__ = counter["original"] + + assert counter["count"] == 1, ( + "PublicKey.decrypt should reuse a single RSAX931Verifier per " + f"instance; got {counter['count']} verifier constructions" + ) + + +def test_privatekey_signer_cached_across_encrypts(_rsa_keypair): + """ + Repeated ``PrivateKey.encrypt`` calls on a single instance must build the + underlying ``RSAX931Signer`` exactly once. Pre-fix behavior was one + signer per encrypt() call. + """ + import salt.utils.rsax931 + + priv = crypt.PrivateKey.from_str(_rsa_keypair["priv_pem"]) + + counter = _count_class_init(salt.utils.rsax931.RSAX931Signer) + try: + for _ in range(50): + priv.encrypt(b"salt") + finally: + salt.utils.rsax931.RSAX931Signer.__init__ = counter["original"] + + assert counter["count"] == 1, ( + "PrivateKey.encrypt should reuse a single RSAX931Signer per " + f"instance; got {counter['count']} signer constructions" + ) + + +def test_pubkey_from_file_returns_cached_instance(_rsa_keypair, _clear_pub_key_cache): + """ + ``PublicKey.from_file`` returns the *same* instance for repeated loads of + the same on-disk file, so downstream libcrypto state (verifiers) is + reused across the entire process. + """ + first = crypt.PublicKey.from_file(_rsa_keypair["pub_path"]) + second = crypt.PublicKey.from_file(_rsa_keypair["pub_path"]) + assert first is second + + +def test_pubkey_from_file_mtime_evicts(_rsa_keypair, _clear_pub_key_cache): + """ + A change to the file's mtime invalidates the cache entry and forces a + fresh ``PublicKey`` instance on the next load. + """ + pub_path = _rsa_keypair["pub_path"] + first = crypt.PublicKey.from_file(pub_path) + # Bump mtime one second into the future. Using an explicit stamp avoids + # relying on filesystem timestamp resolution. + old_mtime = os.path.getmtime(pub_path) + os.utime(pub_path, (old_mtime + 5, old_mtime + 5)) + second = crypt.PublicKey.from_file(pub_path) + assert first is not second + # Same key material -> same underlying cryptography public numbers. + from cryptography.hazmat.primitives.asymmetric import rsa + + assert isinstance(first.key, rsa.RSAPublicKey) + assert isinstance(second.key, rsa.RSAPublicKey) + assert first.key.public_numbers() == second.key.public_numbers() + + +def test_verify_retries_after_rotation_without_mtime_bump( + tmp_path, _clear_pub_key_cache +): + """ + Simulate an on-disk key rotation that preserves mtime (cp -p / NFS mtime + cache / atomic rename). ``PublicKey.verify`` must detect the mismatch, + evict the stale cache entry, and retry once with a freshly loaded key. + """ + stale_priv_pem, stale_pub_pem = crypt.gen_keys(2048) + fresh_priv_pem, fresh_pub_pem = crypt.gen_keys(2048) + + pub_path = tmp_path / "rotated.pub" + pub_path.write_text(stale_pub_pem) + mtime = os.path.getmtime(str(pub_path)) + + # Warm the cache with the stale key. + cached = crypt.PublicKey.from_file(str(pub_path)) + assert (str(pub_path), str(mtime)) in crypt._pub_key_cache + + # Rotate on disk without bumping mtime. A signature produced by the + # fresh key must NOT validate against the cached stale key on the first + # try, but the retry-on-fail path reloads and succeeds. + pub_path.write_text(fresh_pub_pem) + os.utime(str(pub_path), (mtime, mtime)) + + fresh_priv = crypt.PrivateKey.from_str(fresh_priv_pem) + message = b"rotation-safety-check" + signature = fresh_priv.sign(message) + + assert cached.verify(message, signature) is True + # The retry evicts the stale entry and reinstalls a fresh instance for + # the same (path, mtime) key. + assert crypt._pub_key_cache[(str(pub_path), str(mtime))] is not cached + + +def test_decrypt_retries_after_rotation_without_mtime_bump( + tmp_path, _clear_pub_key_cache +): + """ + Mirror of the verify retry, but for ``PublicKey.decrypt`` which drives the + X9.31 padding code path used by AsyncAuth. A payload signed by the + freshly rotated private key must decrypt successfully even though the + cache initially holds the stale public key. + """ + stale_priv_pem, stale_pub_pem = crypt.gen_keys(2048) + fresh_priv_pem, fresh_pub_pem = crypt.gen_keys(2048) + + pub_path = tmp_path / "rotated.pub" + pub_path.write_text(stale_pub_pem) + mtime = os.path.getmtime(str(pub_path)) + + cached = crypt.PublicKey.from_file(str(pub_path)) + + pub_path.write_text(fresh_pub_pem) + os.utime(str(pub_path), (mtime, mtime)) + + fresh_priv = crypt.PrivateKey.from_str(fresh_priv_pem) + signed = fresh_priv.encrypt(b"salt") + + assert cached.decrypt(signed) == b"salt" + + +def test_verify_genuine_bad_sig_returns_false_after_retry( + _rsa_keypair, _clear_pub_key_cache +): + """ + A genuinely invalid signature must still return ``False`` even though the + retry-on-fail path will attempt to reload the key from disk. The retry + is bounded (one extra attempt) and never papers over real failures. + """ + pub = crypt.PublicKey.from_file(_rsa_keypair["pub_path"]) + forged = b"\x00" * 256 + assert pub.verify(b"any message", forged) is False + + +def test_decrypt_genuine_bad_payload_raises_after_retry( + _rsa_keypair, _clear_pub_key_cache +): + """ + ``PublicKey.decrypt`` re-raises the underlying ``ValueError`` for genuine + decryption failures after exactly one retry. This preserves the + pre-cache contract callers rely on. + """ + pub = crypt.PublicKey.from_file(_rsa_keypair["pub_path"]) + with pytest.raises(ValueError): + pub.decrypt(b"\x00" * 256) From cb90db7a81f1978795449210e3e8fa5ba34829a6 Mon Sep 17 00:00:00 2001 From: "Daniel A. Wozniak" Date: Mon, 3 Aug 2026 23:00:56 -0700 Subject: [PATCH 2/2] Fall back to uncached load when getmtime fails in PublicKey.from_file The mtime-keyed cache added in b6b10f8b6a2 calls os.path.getmtime() before opening the key file. That changed the error surface for callers (and tests) that expected FileNotFoundError to come from the salt.utils.files.fopen call path -- most notably tests/pytests/unit/crypt/test_crypt_cryptography.py::test_verify_signature, which mocks fopen to return public-key bytes without actually creating the file on disk. Wrap the getmtime call in try/except OSError and fall through to the uncached BaseKey.from_file path when the mtime probe fails. This preserves the original fopen-first error propagation while keeping the cache behavior intact for the normal on-disk case. Add two regression tests: - from_file on a truly missing path still raises FileNotFoundError. - from_file with fopen mocked but no real file returns a PublicKey via the uncached path and does not populate _pub_key_cache. --- salt/crypt.py | 11 +++++++++- tests/pytests/unit/test_crypt.py | 37 ++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/salt/crypt.py b/salt/crypt.py index 98dfb68e22bb..2a12581b9930 100644 --- a/salt/crypt.py +++ b/salt/crypt.py @@ -414,8 +414,17 @@ def from_file(cls, path, *args, **kwargs): the same key file share a single ``PublicKey`` instance (and therefore a single cached ``RSAX931Verifier``). A key rotation on disk bumps the file's mtime and invalidates the cache automatically. + + If ``os.path.getmtime`` fails (missing file, permission error, or a + test that mocks ``fopen`` without a real file on disk), fall through + to an uncached load so error propagation matches the pre-cache + ``fopen``-first behavior. """ - return _get_pub_key_with_evict(path, str(os.path.getmtime(path))) + try: + mtime = str(os.path.getmtime(path)) + except OSError: + return super().from_file(path, *args, **kwargs) + return _get_pub_key_with_evict(path, mtime) def __init__(self, key_bytes): log.debug("Loading public key") diff --git a/tests/pytests/unit/test_crypt.py b/tests/pytests/unit/test_crypt.py index 274e913d6282..953a058f0884 100644 --- a/tests/pytests/unit/test_crypt.py +++ b/tests/pytests/unit/test_crypt.py @@ -865,3 +865,40 @@ def test_decrypt_genuine_bad_payload_raises_after_retry( pub = crypt.PublicKey.from_file(_rsa_keypair["pub_path"]) with pytest.raises(ValueError): pub.decrypt(b"\x00" * 256) + + +def test_pubkey_from_file_missing_path_raises_from_fopen(_clear_pub_key_cache): + """ + Regression test: ``PublicKey.from_file`` must propagate + ``FileNotFoundError`` from the ``fopen`` layer rather than from the + mtime lookup when the file does not exist. Previously the mtime probe + ran first and callers/tests that expected the ``fopen`` failure mode + (including those that patch ``salt.utils.files.fopen`` alone) saw a + different error shape. + """ + missing = "/does/not/exist/on/disk.pub" + with pytest.raises(FileNotFoundError): + crypt.PublicKey.from_file(missing) + + +def test_pubkey_from_file_uses_fopen_when_mtime_unavailable( + _rsa_keypair, _clear_pub_key_cache +): + """ + Regression test for tests/pytests/unit/crypt/test_crypt_cryptography.py:: + test_verify_signature. Callers may mock ``salt.utils.files.fopen`` to + feed key bytes without ever writing the file to disk. In that case + ``os.path.getmtime`` fails and ``from_file`` must fall back to the + uncached fopen-based load rather than surfacing the mtime error. + """ + import salt.utils.files + + with salt.utils.files.fopen(_rsa_keypair["pub_path"], "rb") as fp: + pub_bytes = fp.read() + with patch("salt.utils.files.fopen", mock_open(read_data=pub_bytes)): + pub = crypt.PublicKey.from_file("/keydir/does-not-exist.pub") + assert isinstance(pub, crypt.PublicKey) + # Uncached path is intentional -- no mtime means no cache key. + assert not any( + path == "/keydir/does-not-exist.pub" for path, _ in crypt._pub_key_cache + )