diff --git a/changelog/69940.fixed.md b/changelog/69940.fixed.md new file mode 100644 index 00000000000..5b9caa05ef8 --- /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 bbdc7b2e248..2a12581b993 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,27 @@ 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. + + 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. + """ + 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") try: @@ -405,6 +434,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 +461,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 +482,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 +526,7 @@ def __init__(self, data, password=None): data.encode(), password=password, ) + self._signer = None # pylint: enable=super-init-not-called @@ -474,6 +538,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 +552,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 691f8970491..953a058f088 100644 --- a/tests/pytests/unit/test_crypt.py +++ b/tests/pytests/unit/test_crypt.py @@ -645,3 +645,260 @@ 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) + + +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 + )