Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,7 @@ Key rules:
- **v2 DTOs**: Use `@field_serializer('field', when_used='json')` to unwrap for JSON wire responses while keeping wrappers in Python-mode `model_dump()`.
- **CLI arguments**: Declare the argument type as `secret` in `cli-reference.yaml`. The generator produces `SecretStr` as the argparse type converter, so the value is wrapped at parse time.
- **Logging**: Never log unwrapped secret values. Response-body logging is gated by `Settings().log_response_bodies` (env `SB_LOG_RESPONSE_BODIES`, default `False`). External libraries that log HTTP bodies (`urllib3`, `kubernetes.client.rest`) are silenced to WARNING. The web access log records only `request.url.path`, never the query string.
- **Downstream of the unwrap**: `services/spdk_http_proxy_server.py` receives JSON-RPC bodies that have already been through `unwrap_secrets_for_send`, so no `SecretStr` survives to mask by. Log those through `redact_rpc_params` from `simplyblock_core/utils/secrets.py`, which masks by parameter name (`SENSITIVE_RPC_PARAMS`). An RPC that carries new key material or a new credential adds its parameter name to that set — masking by type in `rpc_client` alone does not reach the proxy.
- **Comparison**: Use `hmac.compare_digest(secret.get_secret_value(), other)` for timing-safe comparison.
- **Testing**: New secret-bearing code needs masking, wire-delivery, and FDB round-trip tests. See `tests/AGENTS.md` § Secret-handling tests for the required assertions and canonical examples.

Expand Down
4 changes: 3 additions & 1 deletion simplyblock_core/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ Core business logic, data models, and background services for the Simplyblock co
- `models/` — Data models inheriting from `BaseModel` (see below).
- `services/` — Background services for monitoring and async task execution (health checks, snapshot/lvol/storage-node monitors, task runners for backup, migration, restart, etc.).
- `db_controller.py` — Singleton `DBController` wrapping FoundationDB. All data access goes through this class.
- `rpc_client.py` — JSON-RPC client for communicating with storage node SPDK processes. `Session` construction is pooled by `RPCSessionPool` (keyed on identity + retry; `timeout` stays per-call). `services/spdk_http_proxy_server.py`, the receiving end, supports HTTP/1.1 keep-alive so those pooled connections are actually reused end-to-end.
- `rpc_client.py` — JSON-RPC client for communicating with storage node SPDK processes. `Session` construction is pooled by `RPCSessionPool` (keyed on identity + retry; `timeout` stays per-call). `services/spdk_http_proxy_server.py`, the receiving end, supports HTTP/1.1 keep-alive so those pooled connections are actually reused end-to-end. It is a FastAPI app on uvicorn: `create_app()` builds it, importing the module has no side effects, and it exposes a Prometheus endpoint on `/_meta/metrics` (same path as `simplyblock_web`, behind the same basic-auth credentials as the RPCs) alongside a periodic timing summary in its log. Per-request logging follows `simplyblock_web/app.py`: uvicorn's access log is off and an `AccessLogMiddleware` replaces it, enriched with the JSON-RPC method and the id that ties the access line to the request's own `Request:<id>` line.
- `kms/` — Key management abstraction: HashiCorp Vault (`_hcp.py`) and FDB-based (`_fdb.py`) backends.

## Data Model Pattern
Expand Down Expand Up @@ -50,6 +50,8 @@ Clients in `rpc_client.py`, `snode_client.py`, and `fw_api_client.py` accept `Se

Response-body logging is gated by `Settings().log_response_bodies` (default `False`). When off, only status code and content-length are logged.

The request-side `logger.debug` in `_request2` / `_request3` masks by type for the params that are `SecretStr`, and passes every params dict through `redact_rpc_params` (`utils/secrets.py`) to cover the ones that arrive as plain `str` — the v1 API hands controllers raw JSON. The SPDK proxy applies the same redactor, since by the time a body reaches it the wrappers are gone.

## Tests

```bash
Expand Down
4 changes: 3 additions & 1 deletion simplyblock_core/controllers/backup_controller.py
Original file line number Diff line number Diff line change
Expand Up @@ -465,7 +465,9 @@ def restore_backup(backup_id: str, lvol_name: str, pool_id_or_name: str,
else:
crypto_key = None

logger.info(f"Backup allowed hosts: {backup.allowed_hosts}")
logger.info("Backup allowed hosts: %s",
[h["nqn"] if isinstance(h, dict) else h
for h in (backup.allowed_hosts or [])])
lvol_id, error = lvol_controller.add_lvol_ha(
name=lvol_name,
size=size,
Expand Down
10 changes: 7 additions & 3 deletions simplyblock_core/controllers/host_auth.py
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
# coding=utf-8
from pydantic import SecretStr

from simplyblock_core import constants, utils
from simplyblock_core.db_controller import DBController

Expand Down Expand Up @@ -36,8 +38,8 @@ def _register_pool_dhchap_keys_on_node(pool, snode, rpc_client):
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:
continue
Expand Down Expand Up @@ -79,7 +81,9 @@ 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)
# allowed_hosts is a list of plain dicts, so the key material arrives
# untyped; wrap it here or it reaches SNodeClient's request log in clear.
result, error = snode_api.write_key_file(key_name, SecretStr(key_value))
if error:
logger.error("Failed to write key file %s on node %s: %s", key_name, snode.get_id(), error)
continue
Expand Down
8 changes: 6 additions & 2 deletions simplyblock_core/kms/_base.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
from contextlib import AbstractContextManager
from types import TracebackType

from pydantic import SecretStr


class KMS(AbstractContextManager):
def __exit__( # Has to be defined to make the type-checker happy
Expand All @@ -19,11 +21,13 @@ def create_data_encryption_keys(self, path: str, kek_name: str) -> None:
raise NotImplementedError

@abstractmethod
def import_data_encryption_keys(self, path: str, kek_name: str, keys: tuple[str, str]) -> None:
def import_data_encryption_keys(
self, path: str, kek_name: str, keys: tuple[SecretStr, SecretStr],
) -> None:
pass

@abstractmethod
def get_data_encryption_keys(self, path: str, kek_name: str) -> tuple[str, str]:
def get_data_encryption_keys(self, path: str, kek_name: str) -> tuple[SecretStr, SecretStr]:
pass

@abstractmethod
Expand Down
20 changes: 15 additions & 5 deletions simplyblock_core/kms/_fdb.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import json

from pydantic import SecretStr

from simplyblock_core.db_controller import DBController
from simplyblock_core.models.cluster import Cluster
from simplyblock_core.utils import generate_hex_string
Expand All @@ -21,18 +23,26 @@ def _key(path: str) -> bytes:

def create_data_encryption_keys(self, path: str, kek_name: str) -> None:
self.import_data_encryption_keys(
path, kek_name, (generate_hex_string(32), generate_hex_string(32)),
path, kek_name,
(SecretStr(generate_hex_string(32)), SecretStr(generate_hex_string(32))),
)

def import_data_encryption_keys(self, path: str, kek_name: str, keys: tuple[str, str]) -> None:
self._kv_store.set(self._key(path), json.dumps(list(keys)).encode())
def import_data_encryption_keys(
self, path: str, kek_name: str, keys: tuple[SecretStr, SecretStr],
) -> None:
# The persistence boundary, where plaintext is the stored form —
# the same exception ``BaseModel.write_to_db`` makes.
self._kv_store.set(
self._key(path),
json.dumps([key.get_secret_value() for key in keys]).encode(),
)

def get_data_encryption_keys(self, path: str, kek_name: str) -> tuple[str, str]:
def get_data_encryption_keys(self, path: str, kek_name: str) -> tuple[SecretStr, SecretStr]:
raw = self._kv_store.get(self._key(path))
if not raw:
raise KMSException(f"No keys found at {path}")
key1, key2 = json.loads(raw)
return key1, key2
return SecretStr(key1), SecretStr(key2)

def delete_data_encryption_keys(self, path: str) -> None:
self._kv_store.clear(self._key(path))
Expand Down
16 changes: 10 additions & 6 deletions simplyblock_core/kms/_hcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import hvac
import hvac.exceptions
from pydantic import SecretStr

from ._base import KMS
from ._exceptions import KMSException
Expand Down Expand Up @@ -50,23 +51,24 @@ def _create_data_encryption_key(self, kek_name: str) -> str:
except hvac.exceptions.VaultError as e:
raise KMSException("Request failed") from e

def _encrypt(self, kek_name: str, plaintext_hex: str) -> str:
plaintext_b64 = base64.b64encode(bytes.fromhex(plaintext_hex)).decode()
def _encrypt(self, kek_name: str, plaintext_hex: SecretStr) -> str:
plaintext_b64 = base64.b64encode(
bytes.fromhex(plaintext_hex.get_secret_value())).decode()
try:
return self.client.secrets.transit.encrypt_data(
name=kek_name, plaintext=plaintext_b64, mount_point=self.transit_mount,
)['data']['ciphertext']
except hvac.exceptions.VaultError as e:
raise KMSException("Request failed") from e

def _decrypt(self, kek_name: str, ciphertext: str) -> str:
def _decrypt(self, kek_name: str, ciphertext: str) -> SecretStr:
try:
plaintext_b64 = self.client.secrets.transit.decrypt_data(
name=kek_name, ciphertext=ciphertext, mount_point=self.transit_mount,
)['data']['plaintext']
except hvac.exceptions.VaultError as e:
raise KMSException("Request failed") from e
return base64.b64decode(plaintext_b64).hex()
return SecretStr(base64.b64decode(plaintext_b64).hex())

def create_data_encryption_keys(self, path: str, kek_name: str) -> None:
try:
Expand All @@ -81,7 +83,9 @@ def create_data_encryption_keys(self, path: str, kek_name: str) -> None:
except hvac.exceptions.VaultError as e:
raise KMSException("Request failed") from e

def import_data_encryption_keys(self, path: str, kek_name: str, keys: tuple[str, str]) -> None:
def import_data_encryption_keys(
self, path: str, kek_name: str, keys: tuple[SecretStr, SecretStr],
) -> None:
try:
self.client.secrets.kv.v2.create_or_update_secret(
path=path,
Expand All @@ -94,7 +98,7 @@ def import_data_encryption_keys(self, path: str, kek_name: str, keys: tuple[str,
except hvac.exceptions.VaultError as e:
raise KMSException("Request failed") from e

def get_data_encryption_keys(self, path: str, kek_name: str) -> tuple[str, str]:
def get_data_encryption_keys(self, path: str, kek_name: str) -> tuple[SecretStr, SecretStr]:
try:
encrypted_key1, encrypted_key2 = self.client.secrets.kv.v2.read_secret_version(
path=path,
Expand Down
12 changes: 6 additions & 6 deletions simplyblock_core/rpc_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from simplyblock_core import utils, constants
from simplyblock_core.settings import Settings
from simplyblock_core.utils.helpers import single_or_none
from simplyblock_core.utils.secrets import unwrap_secrets_for_send
from simplyblock_core.utils.secrets import redact_rpc_params, unwrap_secrets_for_send

logger = utils.get_logger()

Expand Down Expand Up @@ -293,7 +293,8 @@ def _request2(self, method, params=None, request_timeout=None):
# window, where a single attach has to land within hundreds of ms).
effective_timeout = request_timeout if request_timeout is not None else self.timeout
try:
logger.debug("From: %s, Requesting method: %s, params: %s", self.host, method, params)
logger.debug("From: %s, Requesting method: %s, params: %s",
self.host, method, redact_rpc_params(params))
Comment thread
mxsrc marked this conversation as resolved.
Dismissed
# Tell the SPDK proxy how long we are willing to wait, so it bounds
# its own SPDK round-trip (and the semaphore slot it holds) to this
# instead of the proxy-global timeout. Prevents an abandoned/stuck
Expand Down Expand Up @@ -336,7 +337,7 @@ def _request2(self, method, params=None, request_timeout=None):
return None, None

def _request3(self, method: str, **kwargs):
logger.debug("Requesting method: %s, params: %s", method, kwargs)
logger.debug("Requesting method: %s, params: %s", method, redact_rpc_params(kwargs))
Comment thread
mxsrc marked this conversation as resolved.
Dismissed
wire_payload = unwrap_secrets_for_send({
'id': 1,
'method': method,
Expand Down Expand Up @@ -826,8 +827,7 @@ def lvol_crypto_create(self, name, base_name, key_name):
}
return self._request("bdev_crypto_create", params)

def lvol_crypto_key_create(self, name, key, key2):
# todo: mask the keys so that they don't show up in logs
def lvol_crypto_key_create(self, name, key: SecretStr, key2: SecretStr):
params = {
"cipher": "AES_XTS",
"key": key,
Expand Down Expand Up @@ -2143,7 +2143,7 @@ def bdev_lvol_batch_transfer_final_step(self, lvol_names, lvol_ids, snapshot_nam

def bdev_s3_create(self, name, secondary_target=0, with_compression=False,
snapshot_backups=True, local_testing=False, local_endpoint="",
access_key_id="", secret_access_key="",
access_key_id="", secret_access_key: Optional[SecretStr] = None,
bdb_lcpu_mask=0, s3_lcpu_mask=0, s3_thread_pool_size=0):
"""Create the S3 bdev device.
Must be called before bdev_lvol_s3_bdev to attach it to an lvstore.
Expand Down
7 changes: 4 additions & 3 deletions simplyblock_core/services/__main__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,10 @@
depend on a file layout, so consumers can migrate off the paths.

``runpy`` rather than importing and calling ``main()``: it reproduces the
semantics of running the file directly, including for the modules whose body
is at import time (see ``spdk_http_proxy_server``), so both invocations behave
identically.
semantics of running the file directly -- ``__name__ == "__main__"``, so a
module's own entry-point guard is what runs, and ``sys.argv[0]`` set to the
module's path -- so both invocations behave identically without this dispatcher
having to assume every service spells its entry point ``main()``.
"""
import argparse
import importlib
Expand Down
Loading
Loading