Skip to content

Cache RSA verifier/signer bridge objects and mtime-key PublicKey.from_file - #69942

Open
dwoz wants to merge 2 commits into
saltstack:3008.xfrom
dwoz:dwoz/perf/pubkey-cache-3008x
Open

Cache RSA verifier/signer bridge objects and mtime-key PublicKey.from_file#69942
dwoz wants to merge 2 commits into
saltstack:3008.xfrom
dwoz:dwoz/perf/pubkey-cache-3008x

Conversation

@dwoz

@dwoz dwoz commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes #69940.

Eliminates RSA public-key + libcrypto bridge-object churn in salt/crypt.py
via three layers of caching:

  • Layer 1 - per-instance bridge cache. Lazy _verifier on PublicKey
    and _signer on PrivateKey. self.key is immutable after __init__,
    so the derived RSAX931Verifier / RSAX931Signer (libcrypto BIO +
    RSA allocations, PEM parse) can be reused for the lifetime of the
    instance.
  • Layer 2 - path-level cache on PublicKey.from_file. Keyed on
    (path, mtime) mirroring the 3006.x _get_key_with_evict idiom. A key
    rotation on disk bumps mtime and invalidates the cache automatically. The
    private-key side (get_rsa_key) is separately being restored on 3008.x
    by a sibling PR and is intentionally untouched here.
  • Layer 3 - retry-on-verify-fail. PublicKey.verify / .decrypt fall
    back to a bounded reload-and-retry when a cached key fails to validate.
    Preserves the pre-cache "always fresh" semantics for the corner cases
    where a rotation preserves mtime (cp -p, NFS mtime cache, atomic
    rename with preserved timestamps). Genuine bad signatures still return
    False / re-raise ValueError and cost only one extra file read + PEM
    parse per forged attempt.

Rotation safety

The pre-cache contract was: every PublicKey.from_file returns a freshly
parsed key. Any rotation on disk between calls is visible immediately. The
three layers keep the same guarantee via two independent mechanisms:

  1. mtime-keyed cache handles the common case (any writer that bumps mtime).
  2. retry-on-fail handles the mtime-preserving edge cases by evicting the
    stale cache entry and reloading once from disk. Bounded to one retry so
    forged signatures cannot induce a loop.

Note: AsyncAuth._auth_singleton_key is a separate cache keyed on
opts["pki_dir"] + io_loop for authenticated session state; it does not
overlap with the new path-level public-key cache, which is keyed on the
absolute filesystem path + mtime of the PEM file.

Empirical validation

Standalone repro under agents/scratch/repro_pubkey_verifier_churn.py
instantiates one PublicKey and calls .decrypt(signed) 1000 times with
RSAX931Verifier.__init__ instrumented:

Baseline (3008.x HEAD):
    PublicKey.decrypt calls: 1000    RSAX931Verifier.__init__ calls: 1000
    RESULT: leak still present (1000 verifiers for 1 instance).

Patched:
    PublicKey.decrypt calls: 1000    RSAX931Verifier.__init__ calls: 1
    RESULT: fix confirmed (1 verifier for 1 instance).

Test plan

  • tests/pytests/unit/test_crypt.py full run: 23 passed, 6 skipped
  • tests/pytests/unit/test_auth.py + tests/pytests/unit/channel/:
    66 passed
  • New tests:
    • test_publickey_verifier_cached_across_decrypts - 50 decrypts trigger 1
      verifier construction (was 50 pre-fix)
    • test_privatekey_signer_cached_across_encrypts - same for signer
    • test_pubkey_from_file_returns_cached_instance - identity check
    • test_pubkey_from_file_mtime_evicts - os.utime bump forces fresh
      instance with matching public numbers
    • test_verify_retries_after_rotation_without_mtime_bump - stale cache
      entry + mtime-preserving rotation + valid signature = retry succeeds
    • test_decrypt_retries_after_rotation_without_mtime_bump - mirror for
      the X9.31 decrypt path used by AsyncAuth
    • test_verify_genuine_bad_sig_returns_false_after_retry - retry never
      papers over real failures
    • test_decrypt_genuine_bad_payload_raises_after_retry - preserves
      ValueError contract

Fixes

Fixes #69940

…_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 saltstack#69940
The mtime-keyed cache added in b6b10f8 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.
@dwoz

dwoz commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

CI fix pushed: cb90db7a81f.

Failed test: tests/pytests/unit/crypt/test_crypt_cryptography.py::test_verify_signature on every unit zeromq 2 job.

Root cause: the new mtime-keyed PublicKey.from_file calls os.path.getmtime(path) before salt.utils.files.fopen. The test patches fopen alone to feed key bytes for /keydir/keyname.pub, so getmtime now raises FileNotFoundError where the pre-cache code raised from fopen (which was mocked -> succeeded).

Fix: wrap the getmtime call in try/except OSError and fall through to the uncached BaseKey.from_file path on failure. Preserves cache behavior for real on-disk files; preserves original fopen-first error surface for missing files and for tests that mock fopen without touching disk.

Regression tests added:

  • test_pubkey_from_file_missing_path_raises_from_fopen — truly missing path still raises FileNotFoundError.
  • test_pubkey_from_file_uses_fopen_when_mtime_unavailable — mocked fopen + non-existent path returns a PublicKey via the uncached path.

Local verification: venv310/bin/pytest tests/pytests/unit/crypt/ tests/pytests/unit/test_crypt.py — 54 passed, 11 skipped (FIPS). Pre-commit clean.

Comment thread salt/crypt.py
if cached is instance:
_pub_key_cache.pop(cache_key, None)
_pub_key_cache_path_index.pop(path, None)
try:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If _pub_key_cache grows over time due to mtime changes replacing key instances without explicit clearing, _pub_key_cache_path_index keeps growing unless an eviction/retry event is triggered. However, given that public keys on a Salt Master are typically limited to minion key directories, memory impact should remain minimal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants