3008.x: RequestRouter._extract_command allocates fresh MasterKeys per RSA request, leaking OpenSSL EVP_PKEY handles
Nobody has claimed this yet.
- Dominant language
- Python
- Stars
- 15.7k
- Forks
- 5.6k
- Avg merge
- 2d 44m
- Merged PRs (30d)
- 80
Description
Description
On 3008.x the pooled master request path routes every incoming payload through RequestRouter._extract_command (salt/master.py:1650-1703). The RSA branch of that method constructs a fresh salt.crypt.MasterKeys per call:
elif enc == "pub":
# RSA encryption
import salt.crypt
mkey = salt.crypt.MasterKeys(self.opts) # every RSA payload
load = mkey.priv_decrypt(load)
MasterKeys.__init__ (salt/crypt.py:530-584) is heavy:
- Constructs a
salt.cache.Cache(opts, driver=self.opts["keys.cache_driver"])object graph. - Calls
_setup_keys()->find_or_create_keys(...)which readsmaster.pemfrom disk and loads the private RSA key into an OpenSSLEVP_PKEYhandle viacryptography.
RSA-encrypted payloads on this path are _auth requests. With open_mode: True + auto_accept: True, or under any workload where minions churn re-authentications (test suites, unstable networks, mass minion restarts), every _auth allocates a fresh MasterKeys on the Python heap and an EVP_PKEY handle that OpenSSL retains in its own arena (not tracked by pymalloc, so it appears as smem "system" growth rather than Python allocator growth).
Impact
This is fix #2 in the sequence identified by the memray-backed audit at agents/reports/zmq-master-app-leak-audit.md in the internal stress rig (tests/monitoring/). Suspect #1 (per-request Crypticle) is tracked in #69922; the ZMQ identity slot-cap is #69920; the warn_until memoize is #69924.
Under RSA-heavy auth churn the per-call cost is ~100x higher than the AES path because the RSA key load is unavoidably a disk read plus an OpenSSL EVP_PKEY parse. Even at low request rates (a handful of _auth/s), the leaked EVP_PKEY handles accumulate in OpenSSL's own arena and show up as monotonic RSS growth in the MWorkerQueue process.
Proposed fix
Hoist the MasterKeys(self.opts) construction out of _extract_command and into RequestRouter.__init__, exposed via a lazy @property so routers that only ever see AES traffic don't pay for the allocation.
class RequestRouter:
def __init__(self, opts, secrets=None):
...
self._master_keys = None
@property
def master_keys(self):
if self._master_keys is None:
import salt.crypt
self._master_keys = salt.crypt.MasterKeys(self.opts)
return self._master_keys
def _extract_command(self, payload):
...
elif enc == "pub":
load = self.master_keys.priv_decrypt(load)
Safety
MasterKeys is immutable once initialized. _setup_keys runs exactly once inside __init__, and no method mutates self.key, self.master_key, self.pub_signature, self.pubkey_signature, or the cache handle after construction. Every other MasterKeys(self.opts) call site in the codebase is inside an __init__ (the master process, AuthFuncs, ReqServerChannel, etc.), i.e. one-shot at process startup -- no rotation flow re-instantiates MasterKeys at runtime.
Runtime key rotation goes through SMaster.secrets shared memory (the AES symmetric key), which is orthogonal to the RSA private key. The rotate_aes_key opt drives SMaster.rotate_secrets on secrets["aes"], not MasterKeys.
Reproduction
Stress rig at tests/monitoring/ (already checked in). See agents/reports/zmq-master-app-leak-audit.md for the memray methodology.
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- Open a pull request that references the issue number.
Research direction
Start in salt/master.py:1650-1703 and compare RequestRouter._extract_command with RequestRouter.init, then read MasterKeys initialization in salt/crypt.py:530-584. Run the stress coverage in tests/monitoring/ and consult agents/reports/zmq-master-app-leak-audit.md; done means RSA requests reuse one router-owned MasterKeys instance while AES-only routers remain lazy and the observed handle or RSS growth is addressed.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, performance
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 76/100