Avoid leaking DHCHAP keys to the host system

simplyblock_core/controllers/host_auth.py

@@ -1,6 +1,10 @@
 # coding=utf-8
+from pydantic import SecretStr
+
 from simplyblock_core import constants, utils
 from simplyblock_core.db_controller import DBController
+from simplyblock_core.rpc_client import RPCException
+from simplyblock_core.snode_client import SNodeClientException
 
 logger = utils.get_logger(__name__)
 
@@ -22,55 +26,85 @@ def _get_dhchap_group(cluster, pool=None):
     return "null"
 
 
+def _register_key_on_node(snode, rpc_client, key_name, key_value):
+    """Register a single key in SPDK's keyring on a storage node.
+
+    Prefers the spdk-proxy's keyring_add_key interceptor, which delivers the
+    key material inline and keeps it on a memory-backed volume. A proxy that
+    predates interception support forwards keyring_add_key to SPDK, which
+    rejects the unknown method (-32601); we then fall back to the deprecated
+    SNodeAPI write_key_file + keyring_file_add_key path (key file on persistent
+    host storage) so nodes not yet restarted after an upgrade keep working.
+
+    "File exists" (code -17) means the key is already registered, which is
+    fine (e.g. same host on another volume) and handled via allow_existing.
+
+    Returns True if the key is registered (or already was), False on failure.
+    """
+    try:
+        rpc_client.keyring_add_key(key_name, key_value, allow_existing=True)
+        return True
+    except RPCException as e:
+        if e.code != -32601:  # anything but "Method not found" is a real failure
+            logger.error("Failed to register key %s in SPDK keyring on node %s: %s",
+                         key_name, snode.get_id(), e.message)
+            return False
+        logger.warning("Node %s uses a spdk-proxy without keyring_add_key support; "
+                       "falling back to the deprecated on-disk key path — restart "
+                       "the storage node to enable in-memory key delivery",
+                       snode.get_id())
+
+    try:
+        key_path, error = snode.client().write_key_file(key_name, key_value)
+    except SNodeClientException as e:
+        logger.error("Failed to write key file %s on node %s: %s",
+                     key_name, snode.get_id(), e.message)
+        return False
+    if error:
+        logger.error("Failed to write key file %s on node %s: %s",
+                     key_name, snode.get_id(), error)
+        return False
+    try:
+        rpc_client.keyring_file_add_key(key_name, key_path, allow_existing=True)
+        return True
+    except RPCException as e:
+        logger.error("Failed to register key %s in SPDK keyring on node %s: %s",
+                     key_name, snode.get_id(), e.message)
+        return False
+
+
 def _register_pool_dhchap_keys_on_node(pool, snode, rpc_client):
-    """Write pool-level DHCHAP key files to a storage node and register in SPDK keyring.
+    """Register pool-level DHCHAP keys on a storage node's SPDK keyring.
 
     All LVols in a DHCHAP pool share one key pair stored on the pool.
     Key names are pool-scoped so a single registration serves all LVols.
 
     Returns a dict with 'dhchap_key' and 'dhchap_ctrlr_key' keyring names,
     or an empty dict on failure.
     """
-    snode_api = snode.client()
     safe_pool = pool.get_id().replace("-", "_")
     key_names = {}
 
     for key_type, key_value in (
-        ("dhchap_key", pool.dhchap_key.get_secret_value()),
-        ("dhchap_ctrlr_key", pool.dhchap_ctrlr_key.get_secret_value()),
+        ("dhchap_key", pool.dhchap_key),
+        ("dhchap_ctrlr_key", pool.dhchap_ctrlr_key),
     ):
-        if not key_value:
+        if not key_value or not key_value.get_secret_value():
             continue
         key_name = f"pool_{safe_pool}_{key_type}"
-        result, error = snode_api.write_key_file(key_name, key_value)
-        if error:
-            logger.error("Failed to write pool key %s on node %s: %s",
-                         key_name, snode.get_id(), error)
-            continue
-        key_path = result
-        ret, err = rpc_client._request2("keyring_file_add_key",
-                                        {"name": key_name, "path": key_path})
-        if not ret and err:
-            if err.get("code") == -17:
-                logger.info("Pool key %s already in SPDK keyring on node %s, reusing",
-                            key_name, snode.get_id())
-            else:
-                logger.error("Failed to register pool key %s in SPDK keyring on node %s: %s",
-                             key_name, snode.get_id(), err.get("message", err))
-                continue
-        key_names[key_type] = key_name
+        if _register_key_on_node(snode, rpc_client, key_name, key_value):
+            key_names[key_type] = key_name
 
     return key_names
 
 
 def _register_dhchap_keys_on_node(snode, host_nqn, host_entry, rpc_client):
-    """Write DHCHAP key files to a storage node and register them in SPDK's keyring.
+    """Register per-host DHCHAP/PSK keys on a storage node's SPDK keyring.
 
     Returns a dict mapping key type ('dhchap_key', 'dhchap_ctrlr_key', 'psk')
     to the SPDK keyring name for use in subsystem_add_host.
     """
-    snode_api = snode.client()
-    # Sanitize host NQN for use as filename
+    # Sanitize host NQN for use as key name
     safe_host = host_nqn.replace(":", "_").replace(".", "_")
     key_names = {}
 
@@ -79,24 +113,8 @@ def _register_dhchap_keys_on_node(snode, host_nqn, host_entry, rpc_client):
         if not key_value:
             continue
         key_name = f"{key_type}_{safe_host}"
-        result, error = snode_api.write_key_file(key_name, key_value)
-        if error:
-            logger.error("Failed to write key file %s on node %s: %s", key_name, snode.get_id(), error)
-            continue
-        key_path = result
-        # Register in SPDK keyring — "File exists" (code -17) means the key
-        # is already registered, which is fine (e.g. same host on another volume).
-        ret, err = rpc_client._request2("keyring_file_add_key",
-                                        {"name": key_name, "path": key_path})
-        if not ret and err:
-            if err.get("code") == -17:
-                logger.info("Key %s already in SPDK keyring on node %s, reusing",
-                            key_name, snode.get_id())
-            else:
-                logger.error("Failed to register key %s in SPDK keyring on node %s: %s",
-                             key_name, snode.get_id(), err.get("message", err))
-                continue
-        key_names[key_type] = key_name
+        if _register_key_on_node(snode, rpc_client, key_name, SecretStr(key_value)):
+            key_names[key_type] = key_name
 
     return key_names
 

simplyblock_core/rpc_client.py

@@ -178,7 +178,7 @@
         return None, None
 
     def _request3(self, method: str, **kwargs):
         logger.debug("Requesting method: %s, params: %s", method, kwargs)
         try:
             wire_payload = unwrap_secrets_for_send({
                 'id': 1,
@@ -235,6 +235,11 @@
     def keyring_file_add_key(self, name: str, path: str, *, allow_existing: bool = False):
         """Register a file-based key in SPDK's keyring by path.
 
+        Deprecated: requires the key file to be placed on the node beforehand
+        (via the deprecated SNodeAPI write_key_file endpoint). Use
+        keyring_add_key instead, which delivers the key material through the
+        spdk-proxy without touching persistent storage.
+
         Args:
             name: Key name for the SPDK keyring.
             path: Path to the key file on the storage node.
@@ -250,6 +255,34 @@
                 return None
             raise
 
+    def keyring_add_key(self, name: str, key: SecretStr, *, allow_existing: bool = False):
+        """Register a key in SPDK's keyring, delivering the material inline.
+
+        Intercepted by the spdk-proxy, which writes the key to a memory-backed
+        secrets directory shared with the SPDK container and forwards a
+        keyring_file_add_key referencing that file. A proxy that predates
+        interception forwards this to SPDK, which rejects the unknown method
+        with -32601; callers fall back to keyring_file_add_key in that case.
+
+        Args:
+            name: Key name for the SPDK keyring.
+            key: The key material (e.g. DHCHAP/PSK key in its wire format).
+            allow_existing: When True, silently succeed if the key is already
+                registered with the same material (SPDK errno -17). When False
+                (default) an existing key raises RPCException.
+
+        Raises:
+            RPCException: On proxy or SPDK errors, including a key of the same
+                name existing with different material.
+        """
+        try:
+            return self._request3("keyring_add_key", name=name, key=key)
+        except RPCException as e:
+            if allow_existing and e.code == -17:
+                logger.debug("Key %s already in SPDK keyring, reusing", name)
+                return None
+            raise
+
     def keyring_file_remove_key(self, name):
         """Remove a key from SPDK's keyring."""
         params = {"name": name}

simplyblock_core/services/spdk_http_proxy_server.py

@@ -4,6 +4,7 @@
 import json
 import logging
 import os
+import re
 import socket
 import sys
 import threading
@@ -204,6 +205,145 @@ def _rpc_call_inner(req, req_data, req_time, sock_timeout):
             pass
 
 
+KEY_NAME_RE = re.compile(r'^[a-zA-Z0-9_-]+$')
+
+
+def _jsonrpc_error(req_id, code, message):
+    return json.dumps({'jsonrpc': '2.0', 'id': req_id, 'error': {'code': code, 'message': message}})
+
+
+def _response_error(response):
+    """Extract the JSON-RPC error object from a raw response string, if any."""
+    if response is None:
+        return None
+    try:
+        parsed = json.loads(response)
+    except ValueError:
+        return None
+    if not isinstance(parsed, dict):
+        return None
+    return parsed.get('error')
+
+
+def _key_path(name):
+    return os.path.join(SPDK_PROXY_KEY_DIR, name)
+
+
+def _remove_key_file(name):
+    try:
+        os.unlink(_key_path(name))
+    except FileNotFoundError:
+        pass  # ignore error for idempotency
+
+
+def _handle_keyring_add_key(req_data, client_timeout):
+    """Virtual method {name, key}: write the key material to the memory-backed
+    secrets dir and forward a rewritten keyring_file_add_key {name, path} to
+    SPDK. Key material must never be forwarded or logged (rpc_call logs params).
+    """
+    req_id = req_data.get('id')
+    params = req_data.get('params') or {}
+    name = params.get('name')
+    key = params.get('key')
+    if not isinstance(name, str) or not KEY_NAME_RE.match(name):
+        return _jsonrpc_error(req_id, -32602, 'Invalid key name')
+    if not isinstance(key, str) or not key or not key.isascii():
+        return _jsonrpc_error(req_id, -32602, 'Invalid key material')
+
+    path = _key_path(name)
+    pre_existing = os.path.exists(path)
+    if pre_existing:
+        with open(path, encoding='ascii') as f:
+            if f.read() != key:
+                return _jsonrpc_error(
+                    req_id, -32000,
+                    f"Key '{name}' already exists with different material; remove it first")
+    else:
+        os.makedirs(SPDK_PROXY_KEY_DIR, mode=0o700, exist_ok=True)
+        fd = os.open(path, os.O_WRONLY | os.O_CREAT | os.O_EXCL, 0o600)
+        try:
+            os.write(fd, key.encode('ascii'))
+        finally:
+            os.close(fd)
+    logger.info(f"Intercepted keyring_add_key for key {name}")
+
+    fwd = {k: v for k, v in req_data.items() if k != 'params'}
+    fwd['method'] = 'keyring_file_add_key'
+    fwd['params'] = {'name': name, 'path': path}
+    try:
+        response = rpc_call(json.dumps(fwd).encode('ascii'), client_timeout)
+    except Exception:
+        # The forward failed outright (e.g. SPDK timeout). A key file freshly
+        # written on behalf of this request must not linger on the shared volume.
+        if not pre_existing:
+            _remove_key_file(name)
+        raise
+
+    error = _response_error(response)
+    if error is not None and error.get('code') != -17 and not pre_existing:
+        # We wrote the key file for this request but SPDK rejected it with
+        # something other than "already exists": never leave an orphaned secret.
+        _remove_key_file(name)
+    # On -17 the key is registered in SPDK, so its backing file must stay: the
+    # client's allow_existing treats -17 as reuse. A pre-existing file with
+    # matching content is likewise kept whether SPDK reports success (keyring
+    # lost, e.g. SPDK restarted while the tmpfs survived) or -17.
+    return response
+
+
+def _handle_keyring_file_remove_key(req_data, client_timeout):
+    """Pass keyring_file_remove_key through to SPDK, then delete the key file
+    so removed keys don't linger in the secrets dir."""
+    params = req_data.get('params') or {}
+    name = params.get('name')
+    response = rpc_call(json.dumps(req_data).encode('ascii'), client_timeout)
+    if isinstance(name, str) and KEY_NAME_RE.match(name):
+        error = _response_error(response)
+        if error is None or error.get('code') == -2:  # removed, or unknown to SPDK
+            _remove_key_file(name)
+    return response
+
+
+def _handle_proxy_get_capabilities(req_data, client_timeout):
+    """Answered locally: lets clients probe whether this proxy supports
+    interception before sending any key material (older proxies forward this
+    to SPDK and return 'Method not found' without logging secrets)."""
+    return json.dumps({
+        'jsonrpc': '2.0',
+        'id': req_data.get('id'),
+        'result': {'interceptors': sorted(RPC_INTERCEPTORS.keys())},
+    })
+
+
+RPC_INTERCEPTORS = {
+    'keyring_add_key': _handle_keyring_add_key,
+    'keyring_file_remove_key': _handle_keyring_file_remove_key,
+    'proxy_get_capabilities': _handle_proxy_get_capabilities,
+}
+
+
+def dispatch_rpc(data_string, client_timeout=None):
+    """Route a JSON-RPC payload through a method interceptor if one is
+    registered, otherwise forward it to SPDK unchanged.
+
+    Interception happens before rpc_call so secret-bearing params are
+    rewritten before any request logging.
+    """
+    try:
+        req_data = json.loads(data_string.decode('ascii'))
+        method = req_data.get('method') if isinstance(req_data, dict) else None
+    except (ValueError, UnicodeDecodeError):
+        return rpc_call(data_string, client_timeout)
+    handler = RPC_INTERCEPTORS.get(method) if isinstance(method, str) else None
+    if handler is None:
+        return rpc_call(data_string, client_timeout)
+    try:
+        return handler(req_data, client_timeout)
+    except OSError as e:
+        logger.error(f"Interceptor for {method} failed: {e}")
+        return _jsonrpc_error(req_data.get('id'), -32000, f'proxy interceptor error: {e}')
+
+
 class ServerHandler(BaseHTTPRequestHandler):
     server_session: list[int] = []
     key = ""
@@ -261,7 +401,7 @@ def do_POST(self):
             logger.info(f"read_line_time_diff: {time_diff}")
             read_line_time_diff[read_line_time_start] = time_diff
             try:
-                response = rpc_call(data_string, self.headers.get('X-RPC-Timeout'))
+                response = dispatch_rpc(data_string, self.headers.get('X-RPC-Timeout'))
                 if response is not None:
                     self.do_HEAD()
                     self.wfile.write(bytes(response.encode(encoding='ascii')))
@@ -304,6 +444,11 @@ def run_server(host, port, user, password, is_threading_enabled=False):
 # instead of being aborted; small enough that abandoned/stuck RPCs free their
 # slot quickly. See _resolve_sock_timeout.
 SPDK_TIMEOUT_MARGIN = float(get_env_var("SPDK_TIMEOUT_MARGIN", is_required=False, default=2))
+# Directory for key files written by the keyring_add_key interceptor. Must be
+# a memory-backed volume shared with the SPDK container so key material never
+# touches persistent storage.
+SPDK_PROXY_KEY_DIR = get_env_var("SPDK_PROXY_KEY_DIR", is_required=False,
+                                 default="/var/run/secrets/simplyblock/keys")
 is_threading_enabled = get_env_var("MULTI_THREADING_ENABLED", is_required=False, default=False)
 server_ip = get_env_var("SERVER_IP", is_required=True, default="")
 rpc_port = get_env_var("RPC_PORT", is_required=True)

simplyblock_core/snode_client.py

@@ -115,7 +115,13 @@ def info(self):
         return self._request("GET", "info")
 
     def write_key_file(self, name, content):
-        """Write a DHCHAP key file on the storage node for SPDK keyring."""
+        """Write a DHCHAP key file on the storage node for SPDK keyring.
+
+        Deprecated: use RPCClient.keyring_add_key, which delivers the key
+        material through the spdk-proxy onto a memory-backed volume instead
+        of persistent host storage. Kept one release as a fallback for
+        proxies without interception support.
+        """
         return self._request("POST", "write_key_file", {"name": name, "content": content})
 
     def read_allowed_list(self):