saltstack / saltstack/salt

3008.x: RequestRouter._extract_command allocates fresh MasterKeys per RSA request, leaking OpenSSL EVP_PKEY handles

Open
#69,927 0 comments 0 reactions 0 assignees View on GitHub

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 reads master.pem from disk and loads the private RSA key into an OpenSSL EVP_PKEY handle via cryptography.

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

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.