Skip to content

Restore mtime-based eviction on get_rsa_key - #69943

Open
dwoz wants to merge 1 commit into
saltstack:3008.xfrom
dwoz:dwoz/fix/get_rsa_key-mtime-3008x
Open

Restore mtime-based eviction on get_rsa_key#69943
dwoz wants to merge 1 commit into
saltstack:3008.xfrom
dwoz:dwoz/fix/get_rsa_key-mtime-3008x

Conversation

@dwoz

@dwoz dwoz commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Restore mtime-based cache eviction on salt.crypt.get_rsa_key so a rotated private key on disk is picked up without a process restart.
  • Reintroduce the _get_key_with_evict(path, timestamp, passphrase) helper (memoized) and thread str(os.path.getmtime(path)) through as the eviction key from get_rsa_key.
  • Restore the eviction-semantics docstring from 3006.x, adapted to the 3008.x class layout that loads via PrivateKey.from_file.
  • Add two regression tests: one that rotates the key on disk and asserts the new material is returned, and one that confirms the memoize still short-circuits when nothing changed.

Investigation notes

The eviction was dropped as an incidental change in the server-side PKI refactor merged as #67799 ("new feature: refactor server-side PKI") by @mattp-, specifically follow-up commit ce9308f126f ("migrate remaining master side pki to cache"). That refactor moved the disk-load logic into PrivateKey.from_file and simplified the outer function to:

@salt.utils.decorators.memoize
def get_rsa_key(path, passphrase):
    return PrivateKey.from_file(path, passphrase).key

The @salt.utils.decorators.memoize decorator is a plain str-keyed dict cache (see salt/utils/decorators/__init__.py::memoize) with no built-in mtime handling. Before the refactor, an inner _get_key_with_evict(path, timestamp, passphrase) was the memoized function and get_rsa_key supplied str(os.path.getmtime(path)) as the eviction argument, so a rotated key file (new mtime) produced a different memoize key and the cached entry was bypassed. Collapsing to a single decorated function dropped that eviction semantic.

The mtime pattern still exists in _auth_singleton_key at salt/crypt.py:893 for the AsyncAuth instance singleton cache, but that is a separate cache and does not compensate for the get_rsa_key regression.

This PR restores the original two-layer pattern with the newer PrivateKey.from_file loader inside the helper. It is intentionally the minimal restoration: no changes to get_rsa_pub_key, PublicKey.decrypt, or PrivateKey.encrypt caching.

Test plan

  • pytest tests/pytests/unit/crypt/ clean locally (31 passed, 5 FIPS-only skipped)
  • New test_get_rsa_key_evicts_on_mtime_change fails on unpatched 3008.x, passes with fix
  • pre-commit run --files salt/crypt.py tests/pytests/unit/crypt/test_crypt_cryptography.py changelog/69941.fixed.md clean
  • Full CI green

Fixes #69941

The server-side PKI refactor (PR saltstack#67799) collapsed the two-layer
memoize helper into a single decorated get_rsa_key(path, passphrase),
dropping the file mtime from the memoize key. salt.utils.decorators.memoize
is a plain str-keyed dict cache with no mtime handling, so a private
key rotated on disk was served stale from the process's in-memory
cache until restart.

Restore the pre-refactor pattern: a memoized _get_key_with_evict(path,
timestamp, passphrase) inner helper called from get_rsa_key with
str(os.path.getmtime(path)) so an mtime bump invalidates the cache
entry and the fresh key is loaded from disk.

Fixes saltstack#69941
@dwoz

dwoz commented Aug 4, 2026

Copy link
Copy Markdown
Contributor Author

@mattp- Please review

@mattp- mattp- left a comment

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.

@dwoz can't approve but lgtm 👍

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.

3 participants